process.env, globalThis, and typeof process

[phryneas]

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: evanw/esbuild#1954 (comment)

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.

[JoviDeCroock]

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 and process.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

[benjie]

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 like jsdom 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.

[phryneas]

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.

[JoviDeCroock]

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.

[phryneas]

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 somethings 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.

[benjie]

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.

[phryneas]

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"?

[benjie]

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 🤷 Changing the default would be a semver major change, so we should not put that into this version.

[phryneas]

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 :/

[JoviDeCroock]

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

[yaacovCR]

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

[benjie]

Can bundlers tree-shake based on the result of function calls?

[phryneas]

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.

[JoviDeCroock]

I created a set of experiments here, to highlight the conclusions:

• node this works by default as process is defined
• vite does no replacement by default, only touches import.meta.env.NODE_ENV/...
  • You can use @rollup/plugin-replace to substitute correctly and cut out the branch
• ESBuild/wrangler replace process.env.NODE_ENV === 'production' by default with false
  • ESBuild can leverage the --define CLI arg to specify NODE_ENV to production but not replace typeof process
  • wrangler tells you to use a custom build like rollup
• next replaces process.env.NODE_ENV by default
  • To replace typeof process you need to define that as an extra

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.

[phryneas]

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? 😅

[JoviDeCroock]

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

[phryneas]

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 the globalThis."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'

[phryneas]

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😅

[JoviDeCroock]

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.

• Code search for globalThis.process.env
• Code search for process.env

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.

[yaacovCR]

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 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.

Currently (not just in #4022), the check is done once on startup to decide what the instanceOf export will be.

graphql-js/src/jsutils/instanceOf.ts

Lines 9 to 15 in d811c97

	export const instanceOf: (value: unknown, constructor: Constructor) => boolean =
	/* c8 ignore next 6 */
	// FIXME: https://github.com/graphql/graphql-js/issues/2317
	globalThis.process != null && globalThis.process.env.NODE_ENV === 'production'
	? function instanceOf(value: unknown, constructor: Constructor): boolean {
	return value instanceof constructor;
	}

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 replace process.env.NODE_ENV === 'production' by default with false

• ESBuild can leverage the --define CLI arg to specify NODE_ENV to production but not replace typeof process
• wrangler tells you to use a custom build like rollup

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....

[JoviDeCroock]

For v16 we merged #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.

[phryneas]

I believe we can close this for now then :)