-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cheapen process.env.NODE_ENV lookups in CommonJS bundles. #7627
Conversation
' NODE_ENV: typeof process === "object"', | ||
' && process.env', | ||
' && process.env.NODE_ENV', | ||
' || "development"', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This conditional logic should allow this code to run safely in environments that do not define a global process
variable, or that define process
but not process.env
or process.env.NODE_ENV
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very interesting fix @benjamn! 👍
@benjamn I've gone ahead and permanently removed |
As I explained in #7523 (comment) and #6660 (review), we are unfortunately stuck with the convention of using
process.env.NODE_ENV
to restrict development-only code from running in production, because JS minifiers (in the React ecosystem, at least) look for that exact expression and replace it with a string literal like"development"
or"production"
, which allows the unused code to be stripped from production bundles, for significant savings in bundle size, and better performance in production (due to running less code).That system works well for client-side code, which tends to be minified in a way that strips out the
process.env.NODE_ENV
expressions, but it doesn't work so well in Node.js, where bundling and minification are not common. To make matters worse, theprocess.env.NODE_ENV
expression is quite expensive to evaluate repeatedly in Node.js, since theprocess.env
object is a wrapper around the actual OS environment, and the variable lookups are not cached.To work around this performance problem in Node.js specifically, this PR uses an immediately-invoked function expression to shadow the global
process
variable within the CommonJS bundles that we build, which are most commonly consumed by Node.js. We don't use any properties ofprocess
orprocess.env
other thanprocess.env.NODE_ENV
, so we can shadow the globalprocess
with a bare-bones stub that includes a snapshot of just that information.I tried defining this
process
stub in a module, so that it could be imported wherever we accessprocess.env
, but there does not seem to be any way to set up this import that leaves the originalprocess.env.NODE_ENV
expressions intact after TypeScript and Rollup compilation. If the stub is defined in@apollo/client/utilities
, for example, the final CommonJS bundles will always be rewritten to useutilities.process.env.NODE_ENV
(or something similar), which breaks dead code elimination. By wrapping the CommonJS bundles in an immediately-invoked function expression, we retain full control over theprocess
parameter, preventing it from being rewritten to something else.In the future, we may want to stub
process
in our ESM code as well, so Node.js can load the ESM code natively, without paying the performance penalty of accessingprocess.env.NODE_ENV
more than once. If we do that, we will also need to strip those imports out before generating the CommonJS bundles, to preventprocess.env.NODE_ENV
expressions from getting rewritten, as described above.Should fix #7523 by achieving the goals of #6660 without interfering with minifier-based dead code elimination. Thanks to @jimrandomh for highlighting this issue and proposing a solution that ultimately led me to this PR.