graphql-js
graphql-js copied to clipboard
`process.env`, `globalThis`, and `typeof process`
This is a writeup to discuss this problem in the GraphQL-WG and the GraphQL-JS-WG. Please give feedback in either of those meetings, or here in the issue, until the next GraphQL-JS-WG meeting, where we want to make a final call on this issue.
Last year, #3887 was released which changed the dev check in src/jsutils/instanceOf.ts
from
process.env.NODE_ENV === 'production'
to
globalThis.process?.env.NODE_ENV === 'production'
shortly followed by #3923, which changed it to
globalThis.process && globalThis.process.env.NODE_ENV === 'production'
as some bundlers were choking on the optional chaining syntax.
Since then, various issues and PRs have been opened to change into various other forms.
I'll try to give an overview over problems, potential solutions and their shortcomings here so we can decide on a way forward.
Problems:
1. accessing process.env.NODE_ENV
directly
There is a bunch of environments (e.g. ESM in the browser), where accessing process.env
directly just crashes, since process
is not a variable.
2. accessing globalThis.process.env.NODE_ENV
: bundler replacement
Some bundlers do a string replacement of process.env.NODE_ENV
, which lead to the code being replaced by invalid JavaScript like globalThis."production"
.
(Afaik, most of these have been reported in the upstream bundlers at this point in time and they have fixed their regular expressions)
3. accessing process.env.NODE_ENV
or globalThis.process.env.NODE_ENV
while testing for process
, without checking if the env
property is set: DOM elements with the id process
If a DOM element with the id
process
exists (e.g. <span id="process">...</span>
), this element will be registered as the global variable process
, so it will be accessible as process
and globalThis.process
- but not have a .env
property, so accessing process.env.FOO
will crash.
4. testing for process
to be present with typeof process
: cannot be tree-shaken
Some bundlers (e.g. esbuild) can only replace statements like foo.bar.baz
, but not statements like typeof foo
as they rely on AST-level replacement and just don't have support for that kind of replacement.
The do not plan to add this to ESBuild: https://github.com/evanw/esbuild/issues/1954#issuecomment-1023315210
Potential solutions:
A) process.env.NODE_ENV === 'production'
This would be a rollback to the original code. It works fine with bundlers, but has problem 1.
B) globalThis.process && globalThis.process.env.NODE_ENV === 'production'
This is the current code. It has problems 2. and 3.
C) globalThis.process && globalThis.process.env && globalThis.process.env.NODE_ENV === 'production'
This is the current code with a fix for 3.
It still has problem 2., but that is mostly solved by bundlers after being out for one year.
D) typeof process && process.env.NODE_ENV === 'production'
A variation of the original code. Problems 3. and 4.
E) typeof process && process.env && process.env.NODE_ENV === 'production'
Another variation of the original. Problem 4.
F) const process = globalThis.process; if (process && process.env && process.env.NODE_ENV)
While this would probably replace fine in some bundlers, other bundlers would detect process
as a local variable and never be able to tree-shake it. A variation of problem 4.
A word on bundler code erasing
It should be noted that most bundlers at this point will automatically erase code inside of process.env.NODE_ENV === 'production'
, but not code inside of globalThis.process.env.NODE_ENV === 'production'
.
That said, every bundler can be configured for either of those (as long as we avoid typeof
), so I would propose not to take that into account too much.
Suggestion
My personal preference would be to go with C - globalThis.process && globalThis.process.env && globalThis.process.env.NODE_ENV === 'production'
.
It will never crash in any environment, and while it is not replaced by most bundlers by default, every bundler can be configured to replace it.
Some bundlers will create invalid JS code from this (blindly string-replacing in the middle of an expression), but that can be considered an upstream bug and has mostly been fixed upstream already.
Outlook
Looking into the future, we will hopefully be able to rely on the development
and production
exports conditions, so in a future version of graphql
, this whole discussion will be mostly obsolete.
This Issue is about finding a short-term fix, not a long-term solution.
For the sake of going with what's supported I would lean towards typeof process !== 'undefined' && process.env.NODE_ENV === 'development'
as this is an established practice by now. I fear that a lot of web-bundles might pull in this branch already and that alternative NODE environments don't leverage this env
pointer, this is sub-optimal for both as currently if they don't define the env
they default to having the slow path in production.
We could make a lot of documentation on how to support different bundlers, how to replace, ... but for the sake of releasing something in the 16 release line that benefits everyone I would very much lean on the side of safe failure.
With safe failure I mean that the development
check will only run if it's explicitly set rather than having to be opted out from. Doing this would give the best experience to users imho because:
- If the environment does not support setting this and the consumer is not bundling we choose the fast branch
- This can be supported by their bundler as most will replace both
typeof process
andprocess.env.NODE_ENV
, doing this will cut it out of the bundle in production (size win)
The consumer of this library gets code that stays fast because it opts in to the production path by default and if their bundler supports it it gets tree-shaken and they get the size win.
This solution does not address the id="process"
thing in the DOM but form what I can tell we don't have any in here that do and that get supported by bundlers.
Reflected the aforementioned changes in https://github.com/graphql/graphql-js/pull/4022/files#r1585993254
Let me throw another solution into the ring:
typeof window === 'undefined' && typeof process === 'object' && process.env.NODE_ENV === 'development'
Specifically this attempts to avoid problem 3.
This solution uses the very traditional Node.js pattern of process.env.NODE_ENV
whilst also guarding against a browser environment via the typeof window
check.
Drawbacks:
- It is possible to define
globalThis.window
in Node.js, and perhaps systems likejsdom
do? - Other server-side JS runtimes might define
globalThis.window
Either way, if globalThis.window
exists we'd just run in production mode which feels failsafe to me.
I don't think this has to be a problem for bundlers/tree shaking; they don't need to replace any typeof
expressions; the expression:
typeof window === 'undefined' && typeof process === 'object' && process.env.NODE_ENV === 'development'
could, through substitution, become:
typeof window === 'undefined' && typeof process === 'object' && false
The expression a && b && false
will always be falsy no matter what the values of a
and b
are.
The only concern that I see here would be that a minifier could argue that a
or b
might have side effects if they contain function calls or property access; but even then you can keep the expression and just replace the truthy branch with void 0
:
typeof window === 'undefined' && typeof process === 'object' && false
? void 0 // < Will never be used, so just replace it with a trivial value
: function instanceOf(value. constructor) {
Whether or not bundlers actually do this I don't know; but I see no reason why they should refuse to on consistency grounds.
The real change in both of your suggestions here is from === 'production'
to === 'development'
.
As @benjie correctly has noted, this would solve most problems (almost doesn't matter which other notation it would be combined with at that point) as all other statements could be "logically eliminated" - but like @JoviDeCroock says, it would change the default behaviour from "development by default" to "production by default".
I'm gonna be honest, I considered that change to be so breaking (users won't get warnings anymore, and we still have a very high risk of a real dual module hazard) that I didn't even entertain the thought of adding it here - but if we're all fine with that, and we're aware of that danger, it could be an additional consideration to add to the mix.
In that case, we could also go with something like
typeof process === 'object' && process.env && process.env.NODE_ENV === 'development'
which would also solve problem 3, and wouldn't need to add additional behaviour based on window
on top.
If we're afraid of bundlers getting confused by side effect, we could still throw some additional magic comments into the mix.
I am personally not a fan of adding process.env
there in the middle as that will make bundlers stop cutting out that piece unless documentation is added for it. The goal of mine was to stick to the two heuristics that most frameworks/bundlers have enabled.
I am personally not a fan of adding
process.env
there in the middle as that will make bundlers stop cutting out that piece unless documentation is added for it. The goal of mine was to stick to the two heuristics that most frameworks/bundlers have enabled.
As @benjie noted, it will end up as something && something && false
, so bundlers would immediately shortcut it to false
, no matter what the something
s are. typeof process
also can't be replaced by some/usually won't be replaced by default, but with this specific logic it doesn't matter.
Technically we have the concern that typeof foo === 'object'
doesn't guarantee that foo
isn't null
. So if we wanted to guard against most reasonable possibilities, we'd use something like:
typeof process === 'object' && process !== null && typeof process.env === 'object' && process.env !== null && process.env.NODE_ENV === 'development'
You could drop the !== null
for brevity if you wanted to.
I've seen situations where checking process.env
can result in errors, but I'd say that that comes down to misconfigured tooling. I agree with @phryneas that the process.env
check in the middle shouldn't matter; but our best bet is for someone to actually test this with the common bundlers and concretely show it one way or the other.
At the point where it doesn't matter from a bundler perspective what everything but the final condition is (to be verified), we could also do
globalThis.process && process.env && process.env.NODE_ENV === 'development'
As for testing this - I think I still have a lot of bundlers set up from when we changed over to globalThis.__DEV__
for Apollo Client. I can give it a shot.
The base question is still: do we think it's a good choice to go from "dev by default" to "prod by default"?
I hadn't intended to change it; thought I had copied it from one of the various PRs or implementations or something but maybe not :shrug: Changing the default would be a semver major change, so we should not put that into this version.
Then we'd be back to the original suggestions - all the suggestions we had in comments swap the undefined
default behaviour.
With undefined
behaving as "development", we can't apply any false
short-circuiting :/
Changing the default is for the best imho and it's also not breaking. Users who properly have configured process.env
are successfully differentiating between development and production here. When folks unsuccessfully switch due to i.e. moving to a non-local environment they are subject to the development route by default which in most cases is undesirable as this would force their graphql interactions to be slower. For the rest of this group, it just crashes.
Anyway, just my two cents, I basically have both of those in PR π
To clarify the PR also has the two things that I feel solve most of the issues
-
typeof process !== 'undefined' && process.env.NODE_ENV === 'production'
is what we refer to as non-breaking and fails to solve the DOM-id issue as well as in environments without process it will always exhibit development warnings -
typeof process === 'object' && process !== null && typeof process.env === 'object' && process.env !== null && process.env.NODE_ENV === 'development'
is what's referred to as breaking because it switches the default to production behavior which solves all issues
what about if we simplify to original behavior, but wrapped in a try/catch block a la
function isProduction(): boolean {
try {
return process.env.NODE_ENV === 'production';
} catch {
return true;
}
}
see #4079
Can bundlers tree-shake based on the result of function calls?
Can bundlers tree-shake based on the result of function calls?
Some, but not all.
I'm gonna be honest, I'm still mostly concerned about esbuild
.
I know that vite does string-replacement on top of esbuild
to get around this, but there are lots of other projects using esbuild
and I'm quite sure that not all of them do that (and honestly, that's probably for the better, seeing how brittle that string replacement is).
That's why I feel so hesitant about typeof process
: I'd rather have something that requires manual configuration in many cases than something that cannot ever be configured for dead code erasure in some ecosystems.
It's not just the bundle size - that check is placed in probably the hottest code path of the whole package (and as it is, it unfortunately only makes sense to place it there). Not having the chance to erase it in production means a serious performance impact.
I created a set of experiments here, to highlight the conclusions:
-
node
this works by default asprocess
is defined -
vite
does no replacement by default, only touchesimport.meta.env.NODE_ENV
/...- You can use
@rollup/plugin-replace
to substitute correctly and cut out the branch
- You can use
-
ESBuild
/wrangler
replaceprocess.env.NODE_ENV === 'production'
by default withfalse
-
ESBuild
can leverage the--define
CLI arg to specifyNODE_ENV
to production but not replacetypeof process
-
wrangler
tells you to use a custom build likerollup
-
-
next
replacesprocess.env.NODE_ENV
by default- To replace
typeof process
you need to define that as an extra
- To replace
My own conclusion from this is that typeof process !== 'undefined' && process.env.NODE_ENV === 'production'
is safe for all users. There is something to be said about defaulting to production rather than development but as you lot are seeing that as a breaking change I will let that sit.
I'd rather have something that requires manual configuration in many cases than something that cannot ever be configured for dead code erasure in some ecosystems.
I really don't agree with this, a library should work by default for our users. Users should be able to choose to optimise the library they choose but having a great first experience is still the most important thing. I am personally a big fan of things that work with zero-config by default.
that check is placed in probably the hottest code path of the whole package (and as it is, it unfortunately only makes sense to place it there). Not having the chance to erase it in production means a serious performance impact.
The fact that we do instanceof
checks is way more harmful for performance compared to a typeof
and two equality checks, even if process.env
is considered megamorphic by the engine. To add to that, if performance is such a big concern we can always hoist this check to the top-level so it's only done when the library is imported as also done in #4022.
Doesn't that leave us with the same conclusion regarding typeof process
, just for different reasons, though?
You're saying you want it to work (so, be erased?) out of the box, and by your experiments typeof process
doesn't erase by default.
I'm saying I want to avoid something that cannot even be erased with configuration, and typeof process
cannot be erased (without adding additonal post-processors) in when we're talking esbuild
.
Or am I missing the point? π
Well, I want everything to work by default without configuration, which leaves typeof process
as the obvious pick. This means that we can't support ESBuild for cutting it out of the build but to me personally that isn't really the end all be all, as currently it doesn't even work properly. Adding the need for a configuration is arguably worse as we then regress even further than we already have. In v17 we can add those needs, I am looking for a fix for all of this. Arguably in v17 we should consider import.meta
either way
Oh, I might not have been clear:
I want it to work (as in, in a production environment, pick "production" at runtime without dead code erasure) by default, too, I just would prefer a solution that would enable the possibility of dead code erasure for everyone, even if it meant potentially more configuration to get to that "perfect" state.
What about this variation of C) that should avoid all the problems from the initial post?
globalThis.process && process.env && process.env.NODE_ENV === 'production'
-
globalThis
would only be in the first part of the statement, so theglobalThis."production"
problem would be avoided. - the second check guards against a differently shaped
process
global - the first check avoids
typeof
, meaning it can still be replaced by all existing tooling
or, I believe this might actually make it a little bit easier for bundlers (but this is unverified gut feeling):
!!globalThis.process && !!process.env && process.env.NODE_ENV === 'production'
I have to admit, I feel a bit like there is still some reason why you focused on the typeof process
above over globalThis.process
or similar that I might just be missing - if there is, I'm really not ignoring it out of ill intent, I'm really just blind - please write it out for me again :)
I don't mean to offend, I probably really just missed a point somewhere and am sometimes a bit tone deafπ
Mainly because it's a whole new standard, the ecosystem has quite settled on process.env
without the whole globalThis
so it would benefit from existing setups. We as an ecosystem are also moving towards a new way with import.meta.env
as well as export mapping to differentiate between production and development. The latter part isn't settled on but it would be quite tedious if now all of a sudden a fourth standard starts popping up.
It's 580 results for globalThis.process.env and 6.6 mil results for process.env π as an extra typeof process results in another 5.4 mil results.
EDIT: I guess this basically comes down to fixing it for v16 with the proposed typeof and NODE_ENV check and for 17 digging deeper into whether import.meta.env
is viable for ESM builds as vite
and others started relying on it and process
for CJS.
It's not just the bundle size - that check is placed in probably the hottest code path of the whole package (and as it is, it unfortunately only makes sense to place it there). Not having the chance to erase it in production means a serious performance impact.
The fact that we do
instanceof
checks is way more harmful for performance compared to atypeof
and two equality checks, even ifprocess.env
is considered megamorphic by the engine. To add to that, if performance is such a big concern we can always hoist this check to the top-level so it's only done when the library is imported as also done in #4022.
Currently (not just in #4022), the check is done once on startup to decide what the instanceOf
export will be.
https://github.com/graphql/graphql-js/blob/d811c97d57d14f579fc546b2f03f783f590c33bb/src/jsutils/instanceOf.ts#L9-L15
Line 9 is a bit tough to read, but it's not defining a function, it's saying the type of instanceOf
is a function.
In general, @JoviDeCroock -- the experiments are super-helpful! In theory, they could be added to our integration-tests.
ESBuild
/wrangler
replaceprocess.env.NODE_ENV === 'production'
by default withfalse
ESBuild
can leverage the--define
CLI arg to specifyNODE_ENV
to production but not replacetypeof process
wrangler
tells you to use a custom build likerollup
esbuild (and therefore wrangler) will apparently replace process.env.NODE_ENV
with true
when the --minify
option is set, which I guess makes sense, although is not super-intuituve, (https://esbuild.github.io/api/#platform) (https://developers.cloudflare.com/workers/wrangler/configuration/#inheritable-keys). esbuild
says that this automatic behavior can be overridden when , process.env.NODE_ENV
but on checking the source code, it's clear that this means when process.env.NODE_ENV
is defined as production
using esbuild
configuration, not when it is defined as an environment variable.
One general thought I had when looking at the experiments is that while it seemed to me at first blush that setting process.env.NODE_ENV
to production
prior to running the bundler should have an effect, after some reflection, it started making sense to me that it did not....
The bundler cannot assume that because it itself is running in "production" mode (as fast as possible) etc. that it should be producing a "production" bundle.
So producing a "production" bundle is going to depend on the individual bundler's options, as well as how each of those dependencies set themselves up to detect production
mode. So, it does not seem terrible that specifying the production bundle requires custom configuration. I think the overall approach of this library prior to the globalThis
pr series had been just to follow react
....
For v16 we merged https://github.com/graphql/graphql-js/pull/4022 which is awaiting a patch release and website deploy. I have updated the minifier experiments to test what we have documented and that behaves well - the result is that we now can safely run in any bundler and have resolutions to optimise GraphQL for production.
I believe we can close this for now then :)