alexreardon
`process` doesn't exist in the browser
In index.ts, it's saying that process doesn't exist in the browser. There should be a check here to make sure typeof process !== 'undefined'.
https://github.com/cssinjs/jss/ relies on tiny-warning and process.env is landing in all builds designed to be used in both browser and node env, while in node it just works, for the browser, this requires extra processing.
I think it is a good strategy to have a default value when the user imports it without processing. Currently, I assume we just crash it.
Not sure what should be the default value though, production or development. I would probably go with development like in node, since process.env was borrowed from node anyways.
But process is not defined in the w3c. Further, JSS doesn't specify it needs to be "further processed", in which case your package is NOT standard. You're relying on webpack-specific behavior that has been ported over to new bundlers when (in my opinion) it should have never been present in the first place.
I've never seen non-standard postprocessing of Javascript be required by a library (other than es6 features, of which process is not one of) in the ~10 years I've been maintaining Node/javascript OSS. This sort of thing has always been opt-in by the end product applications, thus not imposing these requirements on downstream consumers.
This undocumented and non-standard requirement causes headaches with custom packaging script writers and build engineers. We've ditched global variables for a reason.
Please reconsider. The change is trivial and it will allow your library to work in standard browser environments.
I agree this is not good. We need to fix this behavior in tiny-warning and make process.env fully optional
@Qix- would you be up for creating a PR in tiny-warning? I think we will be able to convince the author to merge it. If he refuses, we can move this package somewhere else. cc @TrySound
typeof is wrong solution. The point of statically replaced process.env.NODE_ENV is completely eliminate unnecessary code. typeof process will makes production bundle bloated instead.
I've never seen non-standard postprocessing of Javascript be required by a library (other than es6 features, of which process is not one of) in the ~10 years
https://unpkg.com/browse/[email protected]/index.js https://unpkg.com/browse/[email protected]/index.browser.js
And dozens of other projects. All of them rely on convention which can be handled in all bundlers.
typeof process can be a side effect. It will not be replaced.
https://rollupjs.org/repl/?version=2.38.1&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMmlmJTIwKHR5cGVvZiUyMHByb2Nlc3MlMjAlMjYlMjYlMjBmYWxzZSklMjAlN0IlNUNuJTVDdGNvbnNvbGUubG9nKCdoZWxsbyUyMHdvcmxkJyklNUNuJTdEJTIyJTJDJTIyaXNFbnRyeSUyMiUzQXRydWUlN0QlNUQlMkMlMjJvcHRpb25zJTIyJTNBJTdCJTIyZm9ybWF0JTIyJTNBJTIyZXMlMjIlMkMlMjJuYW1lJTIyJTNBJTIybXlCdW5kbGUlMjIlMkMlMjJhbWQlMjIlM0ElN0IlMjJpZCUyMiUzQSUyMiUyMiU3RCUyQyUyMmdsb2JhbHMlMjIlM0ElN0IlN0QlN0QlMkMlMjJleGFtcGxlJTIyJTNBbnVsbCU3RA==
You can follow nanoid example and provide additional entry points with prod and dev. https://unpkg.com/browse/[email protected]/
Here "exports" configured with custom conditions https://unpkg.com/browse/[email protected]/package.json
I would seriously argue the point that adding a typeof check to make a package standard is "bloat". Further, if I personally used either of the aforementioned packages, I would certainly open similar issues with them as well.
Convention does not trump standards. The world would descend into chaos if that were true.
(EDIT, supplementary thought: convention is fine if it's not critical to the baseline functioning of an application - that isn't the case here, though.)
Pretty sure react's browser build doesn't require user to set this up, because they have a version that can run directly in the browser, how do they do it?
I think react provides a production version which has this variable already replaced for the use directly in the browser.
So the problem we have here is that we don't provide a browser-ready version, all dist versions require a bundler because of the process.env
If you want to use the code directly you always free to specify global process object.
Convention does not trump standards. The world would descend into chaos if that were true.
I agree this can be painful but it works and there is a better solution coming conditional exports like here https://unpkg.com/browse/[email protected]/package.json
I would seriously argue the point that adding a typeof check to make a package standard is "bloat". Further, if I personally used either of the aforementioned packages, I would certainly open similar issues with them as well.
It will eliminate the point of using process.env.NODE_ENV to remove development messages in production. They will stay in production bundle.
@kof Nanoid and react provide development and production modules separately. React re-require them in main entry point, nanoid has separate entry point as main with process.env.NODE_ENV + specifies prod/dev in conditional exports.
So to me, it sounds like @TrySound is advocating for JSS to do a pre-publish build step that does the preprocessing prior to hitting the package registries then. This would also be acceptable, though that's quite a lot of extra mechanics to work around non-standard behavior...
@TrySound how about this?
https://rollupjs.org/repl/?version=2.38.1&shareable=JTdCJTIybW9kdWxlcyUyMiUzQSU1QiU3QiUyMm5hbWUlMjIlM0ElMjJtYWluLmpzJTIyJTJDJTIyY29kZSUyMiUzQSUyMnZhciUyMGlzUHJvZCUzQiUyMCU1Q250cnklN0IlMjBpc1Byb2QlMjAlM0QlMjBwcm9jZXNzLmVudi5zb21ldGhpbmclMjAlM0QlM0QlM0QlMjAncHJvZHVjdGlvbiclN0QlMjBjYXRjaChlKSU3QiU3RCU1Q25pZiUyMCghaXNQcm9kKSUyMCU3QiU1Q24lNUN0Y29uc29sZS5sb2coJ2hlbGxvJTIwd29ybGQnKSU1Q24lN0QlNUNuJTIyJTJDJTIyaXNFbnRyeSUyMiUzQXRydWUlN0QlNUQlMkMlMjJvcHRpb25zJTIyJTNBJTdCJTIyZm9ybWF0JTIyJTNBJTIyZXMlMjIlMkMlMjJuYW1lJTIyJTNBJTIybXlCdW5kbGUlMjIlMkMlMjJhbWQlMjIlM0ElN0IlMjJpZCUyMiUzQSUyMiUyMiU3RCUyQyUyMmdsb2JhbHMlMjIlM0ElN0IlN0QlN0QlMkMlMjJleGFtcGxlJTIyJTNBbnVsbCU3RA==
oh actually, I recall there was a way to mark a statement as side-effect free via a comment for bundlers to be able to remove them
@TrySound you're trying to optimize away a check that can only happen safely at runtime. See my comment in the PR.
@Qix- are you aware that we are inlining that module and whatever is added there is repeated every time it is used? It's not a problem as long as it can be removed from production bundle, but what @TrySound is saying we can't make bundlers remove it if it has a typeof side effect. I wonder if there is some smart trick though to make it happen without going the react or nano-id route, which is quite a lot of work for this.
On the other hand what you might not realize is that we can rely on process.env not just for warnings.
Let's start from the beginning. JSS has a lot of messages which help while in development but bloats user bundle in production. By "bloats" I mean a lot of kb which makes site/app loading longer. So the problem WORTH solving.
Thanks to react process.env.NODE_ENV hack already heavily used in node became quite popular for browser packages. React used it for exact the same reason. Killing long development messages from production.
Initially they had a lot of checks but decided it's easier to provide separate development and preoptimised production bundle. Though index.js conditionally require both depending on process.env.NODE_ENV value.
Since then a few folks complained like you about this convention. The only legal solution here is something non-runtime like conditional exports. It will allow both node and bundler choose the right entry point based on configuration. Though I think it only works in node for now and process.env.NODE_ENV will stay for a while.
Currently the only thing we can do is to provide separate entry points for production and development.
Btw @Qix- why don't you complain about import "tiny-warning", import '@babel/runtime/helpers/esm/extends'. Those are not standard identifiers, right? It's the same convention we stuck with.
Thanks to react process.env.NODE_ENV hack already heavily used in node became quite popular for browser packages. React used it for exact the same reason. Killing long development messages from production.
Webpack did this, not React. Webpack also supports a lot of non-standard behavior, including importing non-script assets as though they were scripts via loaders. It's generally assumed package maintainers strip their packages of any such non-standard behavior prior to publishing the code.
Also, React is not the authority here. Neither is Facebook. Not everyone uses React, I'm not sure why their non-standard hacks are to be considered the "norm".
Btw @Qix- why don't you complain about import "tiny-warning", import '@babel/runtime/helpers/esm/extends'. Those are not standard identifiers, right? It's the same convention we stuck with.
What do you mean they're not standard? Both are standards compliant ecmascript.
Also, React is not the authority here. Neither is Facebook. Not everyone uses React, I'm not sure why their non-standard hacks are to be considered the "norm".
Not norm, but again this is a solution for the problem which is worth solving.
What do you mean they're not standard? Both are standards compliant ecmascript.
Browsers not aware how to resolve such identifiers.
@Qix- question, how did you end up with env variable not replaced? What how do you use the package?
@kof I wrote my own acorn-based bundler for an NW.js application due to needing very specific bundling characteristics within a larger build and packaging pipeline.
That explains why nobody else had this problem, because with all bundlers, they have this replacement in place and if they include directly in the page, minified dist is production ready.
I do agree with you that the current state is suboptimal, but I still wonder if it's worth fixing, because this packages has huge amount of users and this is the first issue about this problem.
Just because every bundler uses it doesn't mean it's OK to do it. It just means every bundler has copied all other non-standard behavior in order to fuel adoption, a real problem the Web community has right now.