Use __DEV__ internally instead of process.env.NODE_ENV.
#8347
Conversation
| '@process.env.NODE_ENV': JSON.stringify('production'), | ||
| '@__DEV__': 'false', |
There was a problem hiding this comment.
Our Filesize CI check is only passing because I went ahead and reconfigured the minifier to replace __DEV__ with false, simulating a production environment.
| // In Node.js, where these CommonJS bundles are most commonly used, | ||
| // the expression process.env.NODE_ENV can be very expensive to | ||
| // evaluate, because process.env is a wrapper for the actual OS | ||
| // environment, and lookups are not cached. We need to preserve the | ||
| // syntax of process.env.NODE_ENV expressions for dead code | ||
| // elimination to work properly, but we can apply our own caching by | ||
| // shadowing the global process variable with a stripped-down object | ||
| // that saves a snapshot of process.env.NODE_ENV when the bundle is | ||
| // first evaluated. If we ever need other process properties, we can | ||
| // add more stubs here. | ||
| intro: '!(function (process) {', | ||
| outro: [ | ||
| '}).call(this, {', | ||
| ' env: {', | ||
| ' NODE_ENV: typeof process === "object"', | ||
| ' && process.env', | ||
| ' && process.env.NODE_ENV', | ||
| ' || "development"', | ||
| ' }', | ||
| '});', | ||
| ].join('\n'), |
There was a problem hiding this comment.
This makes PR #7627 obsolete, because we're no longer using process.env.NODE_ENV internally, even in Node.js (where process is defined globally, but process.env lookups are surprisingly expensive).
| declare global { | ||
| const __DEV__: boolean | undefined; | ||
| } |
There was a problem hiding this comment.
Question: are people (especially folks already using TypeScript with React Native) going to be OK with this global declaration? Is there any way to make it less obtrusive/noticeable?
Anticipating apollographql/apollo-client#8347 landing in Apollo Client v3.4.
|
Here's a recommended way to configure a Create React App application to minify |
benjamn
changed the title
benjamn
changed the title
__DEV__ internally instead of process.env.NODE_ENV.
benjamn
force-pushed
the
try-__DEV__-instead-of-process.env.NODE_ENV
branch
from
f844243
to
023a824
Compare
|
Thanks for working on this @benjamn! There is a risk that people are going to update to 3.4 and not notice the impact to their bundle size until after they’ve deployed to production. To help with this we’ll definitely call things out in the |
See PR #8347 for full explanation.
|
@abdonrd I think I can fix that too, but maybe the tool shouldn't be blindly finding/replacing Edit: If the replace({
preventAssignment: true,
values: {
"@__DEV__": JSON.stringify(false),
},
}),@hwillson @brainkim We should probably collect examples of recommended configurations for different bundlers/minifiers. |
|
@benjamn I'm using the official @rollup/plugin-replace. I just tried the |
|
@abdonrd Seems like a known problem: rollup/plugins#803 (closed but not really solved by rollup/plugins#798). Following the recommendation from @jespertheend, I would suggest switching to |
|
@benjamn I'm not sure if it's a good solution change from to this package... Because looking at the statistics, I imagine that many more people will have this problem.
|
|
I agree we should do what we can to accommodate imperfect but popular tools! However, I would also point out that having 2MM downloads/week can put you in a position of not wanting to change anything, for fear of (temporarily) breaking all those applications, even once you've become aware of a fatal flaw in your approach. I don't know if the maintainers think of this flaw as "fatal" like I do, but they must be aware that string manipulation without actual parsing is always going to produce false positives. We'll get this fixed, but I stand behind my person-to-person advice about switching tools! 🚀 |
|
You are right, thanks @benjamn ! |
|
This is a bit of a shot in the dark, but I'm not sure where to look. Updating my project from 3.3 to 3.4.0-rc.15 breaks my app in production. It works in dev mode, but once built, I'm getting these which essentially kills my app at startup:
I wonder if this might be related to this change? Apologies if I'm far off. |
|
@sebastienbarre I believe I already answered a similar question from you here: #7399 (comment). Would you mind opening a new issue? |
Anticipating apollographql/apollo-client#8347 landing in Apollo Client v3.4.
To import `graphql` (and its internal `instanceOf` function) for the first time with `process.env.NODE_ENV` safely polyfilled, PR #8347 selected a somewhat arbitrary export of the `graphql` package that uses `instanceOf`, specifically the `isType` function exported by `graphql/type/definition`. As revealed by issue #8705, importing `isType` was a bad choice, since it depends on a bunch of other code within the `graphql` package, unnecessarily increasing minified+gzip bundle size by 3.4kB. A better choice is the `Source` constructor, which is already imported thanks to other necessary imports, and also imports/uses `instanceOf`.
Currently, because of the naked reference to process.env.NODE_ENV in
src/jsutils/instanceOf.ts, the `graphql` package must either be loaded
in an environment that supports/polyfills process.env, or minified in a
way that replaces process.env.NODE_ENV expressions with string literals
at build time.
As a concrete example of how this problem can manifest, consider running
the following command in your browser console:
await import("https://cdn.jsdelivr.net/npm/graphql@16.1.0/jsutils/instanceOf.mjs")
Even though instanceOf.mjs is otherwise a valid ECMAScript module,
importing it in a browser throws the following exception:
instanceOf.mjs:13 Uncaught ReferenceError: process is not defined
at instanceOf.mjs:12:3
To make matters worse, polyfilling globalThis.process is not a safe
default option in general, because many programs use the presence of the
globalThis.process object to determine if they're running in Node.js.
In order to allow the `@apollo/client` package to run natively (without
a build step or globalThis.process polyfill) in browsers, we had to come
up with an extensive workaround that temporarily polyfills
globalThis.process.env while importing the `graphql` package, and then
immediately removes the polyfill (if it was previously undefined):
apollographql/apollo-client#8347
This PR is my attempt to make those hacks no longer necessary, by making
the `graphql` package more defensive in the way it detects
process.env.NODE_ENV, but also without interrupting various existing
ways in which process.env.NODE_ENV is exposed/handled/transformed in a
typical web application. In short, I use maybe(() =>
process.env.NODE_ENV) in place of just process.env.NODE_ENV, and
maybe(...) makes things safe.
The maybe(() => ...) utility is a convenient way to evaluate an
arbitrary expression like process.env.NODE_ENV safely in any
environment, even if not all parts of the expression are defined:
1. If process.env.NODE_ENV is defined, the maybe(() =>
process.env.NODE_ENV) expression will correctly infer its type
(string | undefined) and return a value of that type when called.
2. If evaluating process.env.NODE_ENV throws an exception (because
either process or process.env are not defined), the maybe(...)
expression will evaluate safely to undefined.
3. If a build tool replaces process.env.NODE_ENV expressions with string
literals, the resulting expressions maybe(() => "production") or
maybe(() => "development") will work as expected.
Other solutions like `typeof process === "object"` and
process?.env?.NODE_ENV chaining fail to satisfy one or more of these
requirements (especially the last one, given the variety of different
build tools used), and tend to require more code.
As explained in #8266, our use of
process.env.NODE_ENVrequires those expressions to be either replaced with a string literal by a minifier (common convention in the React ecosystem) or polyfilled globally.We stopped polyfilling
process.env.NODE_ENVglobally in thets-invariantpackage in apollographql/invariant-packages#94, but@apollo/clientis still relying onprocess.envinternally, as is thegraphqlpackage. If we want to rid ourselves fully of the drawbacks ofprocess.env.NODE_ENV, we probably ought to stop using it ourselves.Though most developers in the React ecosystem will be using a bundler or minifier that automatically replaces
process.env.NODE_ENVat build time, you may not have the luxury of custom minification when loading@apollo/clientfrom a CDN, which leaves only the option of a globalprocess.envpolyfill, which is problematic because that's how some applications detect if the current code is running Node.js (more details/links in #8266).Instead, I believe we can (and should) stop using
process.env.NODE_ENV, and one of many better alternatives is the boolean__DEV__variable used by React Native and internally by React itself, which is much easier to polyfill, since it's a single variable rather than a nested object.Switching to
__DEV__will initially cause a large bundle size regression (+3.5Kb after minification and gzip), but that's not technically a breaking change (hence acceptable for AC3.4), and it should be straightforward to reconfigure minifiers to replace__DEV__withtrueorfalse(or even justundefined), with sufficient guidance from the release notes.Although we could do some more work to reduce the bundle size impact of blindly switching to
__DEV__, I believe this PR already solves the last remaining hurdles documented in #8266, allowing@apollo/client/core@betato be loaded from an ESM-aware CDN like https://esm.run or https://jspm.io.The default
__DEV__value will betruein unminified, buildless browser environments, but it can be initialized differently by a script/module that runs earlier in the HTML of the page. Ifprocess.env.NODE_ENVis available, it will determine the default value of__DEV__, namely__DEV__ === (process.env.NODE_ENV !== "production"), unless__DEV__is already defined globally or replaced by a minifier, in which case we use it without modification.That still leaves the
process.env.NODE_ENVcheck ingraphql/jsutils/instanceOf.mjs. Discussion in graphql/graphql-js#2894 suggests the plan is to stop usingNODE_ENValtogether, which would be ideal, but that won't happen untilgraphql@16at the earliest.To work around the problem in the meantime, this PR adopts a strategy of polyfilling
process.env.NODE_ENVonly briefly, while importing code that depends ongraphql/jsutils/instanceOf.mjs, and then synchronously removes the global polyfill, so it does not permanently pollute the global namespace, and is never visible to code outside thegraphqlpackage.This strategy assumes
@apollo/clientis the first to import thegraphqlpackage. If you have other code that importsgraphql/jsutils/instanceOf.mjsearlier, and you don't have aprocess.env.NODE_ENVstrategy already, it's (still) your responsibility to make that import work, however you see fit. Apollo Client is only responsible for making sure its own imports of thegraphqlpackage have a chance of succeeding, a responsibility I believe this PR fulfills cleverly/technically, if not exactly elegantly. Of course, this charade does not (need to) happen ifprocess.env.NODE_ENVis already defined or has been minified away, but only in cases when accessingprocess.env.NODE_ENVwould throw aReferenceError, since that's the specific failure we're trying to prevent withingraphql.