avoid using process.env with globalThis

[dimaMachina]

fixes #3918 #3925

globalThis.process and globalThis.window is a syntax that doesn't eliminated by webpack

Input

function TRY() {
  if (process.env.NODE_ENV !== 'production') { // is dev
    console.log(111) // ✅ eliminated
  }
  if (process?.env.NODE_ENV !== 'production') { // is dev
    console.log(222) // ❌ not eliminated
  }
  if (globalThis.process?.env.NODE_ENV !== 'production') { // is dev
    console.log(333) // ❌ not eliminated
  }
  if (typeof process !== 'undefined' && process.env.NODE_ENV !== 'production') { // is dev
    console.log(444) // ✅ eliminated
  }
  if (globalThis.process.env.NODE_ENV !== 'production') { // is dev
    console.log(555) // ❌ not eliminated
  }
  if (typeof window === 'undefined') { // Is server
    console.log(666) // ✅ eliminated
  }
  if (!globalThis.window) { // Is server
    console.log(777) // ❌ not eliminated
  }
  if (typeof process === 'undefined' || process.env.NODE_ENV === 'production') {
    console.log(888) // is prod ✅ kept
  } else {
    console.log(999) // is dev ✅ eliminated
  }
  return 'foobar'
}

Output (by webpack in Next.js)

function u() {
  var e
  return (
    (null == d ? void 0 : 'production') !== 'production' && console.log(222),
    (null === (e = globalThis.process) || void 0 === e
      ? void 0
      : e.env.NODE_ENV) !== 'production' && console.log(333),
    'production' !== globalThis.process.env.NODE_ENV && console.log(555),
    globalThis.window || console.log(777),
    console.log(888),
    'foobar'
  )
}

Conclusion

typeof process === 'undefined' || process.env.NODE_ENV === 'production'

The above code is a perfect case for running in a browser or to eliminate the opossite case (development dead code) by webpack for example.

[netlify]

✅ Deploy Preview for compassionate-pike-271cb3 ready!

Name	Link
🔨 Latest commit	06e2d8a
🔍 Latest deploy log	https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/649ad92a245e680008efa9f2
😎 Deploy Preview	https://deploy-preview-3927--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[github-actions]

Hi @B2o5T, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

• @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
• @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

[phryneas]

The problem using typeof process for this check is that many bundlers that actually look at the code they are replacing won't be able to replace it.
According to my research, esbuild can only replace expressions with variable names, not full code pieces like typeof process.

You should be able to configure webpack manually to replace globalThis.process, but it is not possible to configure esbuild at all to replace typeof process. (See this statement for typeof window)

[dimaMachina]

The problem using typeof process for this check is that many bundlers that actually look at the code they are replacing won't be able to replace it.

this typeof process check is made only for browsers in case of not using bundlers because process is not defined in browsers

process.env.NODE_ENV will be always replaced by esbuild/webpack/vite

You should be able to configure webpack manually to replace globalThis.process, but it is not possible to configure esbuild at all to replace typeof process. (See this statement for evanw/esbuild#1954 (comment))

no need to manually configure webpack, everything should works fine out of box, never saw somebody uses globalThis.process code, this code is not replaced by static analyzers

[phryneas]

@B2o5T If you fix up the logic that was incorrect in your code above, you will end up with code that will not be tree-shaken out if you do not replace the typeof process check.

typeof process !== 'undefined' && process.env.NODE_ENV === 'production'

cannot be tree-shaken, as it is essentially

if (runtimeValue && true) {
  //...
} else {
  //...
}

which comes down to

if (runtimeValue) {
  //...
} else {
  //...
}

It would only tree-shake away in development, never in production.

That's why you need to configure your bundler to also replace the typeof check - which, as I stated, is not possible in some bundlers.

On the other hand, you can easily configure your current webpack installation to replace away the current behaviour. It's just not preconfigured out of the box.

[dimaMachina]

if you fix up the logic that was incorrect in your code above, you will end up with code that will not be tree-shaken out if you do not replace the typeof process check.

my code typeof process === 'undefined' || process.env.NODE_ENV === 'production' is already treeshaked for production environement, I tested in Next.js which uses webpack

Current globalThis.process?.env.NODE_ENV has much more problems than my fix

• not treeshaked (tested in Next.js)
• produce issues in vite
• will execute dev opposite code in browsers that don't use bundlers

[dimaMachina]

@B2o5T If you fix up the logic that was incorrect in your code above, you will end up with code that will not be tree-shaken out if you do not replace the typeof process check.

typeof process !== 'undefined' && process.env.NODE_ENV === 'production'

cannot be tree-shaken, as it is essentially

if (runtimeValue && true) {
  //...
} else {
  //...
}

which comes down to

if (runtimeValue) {
  //...
} else {
  //...
}

It would only tree-shake away in development, never in production.

That's why you need to configure your bundler to also replace the typeof check - which, as I stated, is not possible in some bundlers.

On the other hand, you can easily configure your current webpack installation to replace away the current behaviour. It's just not preconfigured out of the box.

I don't see the logic, please take a look of my fix that uses different code

typeof process === 'undefined' || process.env.NODE_ENV === 'production'

CAN be tree-shaken,

if (runtimeValue || true) {
  //...
} else {
  //...
}

which comes down to - always truthy

if (runtimeValue || true) {
  //...
}

[phryneas]

typeof process === 'undefined' || process.env.NODE_ENV === 'production'

As I already said multiple times, that is a completely different logic than before your change. It will not give dev warnings if NODE_ENV is not set, which was the case before.
You cannot just sneak a behavior change in as a "fix".

Current globalThis.process?.env.NODE_ENV has much more problems than my fix

That is not the current code, you are operating on an outdated branch.

[dimaMachina]

Current globalThis.process?.env.NODE_ENV has much more problems than my fix

That is not the current code, you are operating on an outdated branch.

it's a code from the main branch, why it's outdated?

graphql-js/src/jsutils/instanceOf.ts

Line 12 in d766c8e

globalThis.process?.env.NODE_ENV === 'production'

or

https://github.com/graphql/graphql-js/blob/main/src/jsutils/instanceOf.ts#L12

[phryneas]

For reference: This is the code before globalThis was introduced. It is intended that this executed the dev check if nothing is set.

graphql-js/src/jsutils/instanceOf.ts

Lines 9 to 18 in 0926554

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

[phryneas]

it's a code from the main branch, why it's outdated?

Because we are talking about Version 16 here. The main branch targets v17 and is currently up to date with this change from 16.7.0, but not yet with 16.7.1:

https://github.com/graphql/graphql-js/blob/16.x.x/src/jsutils/instanceOf.ts#L12

[dimaMachina]

As I already said multiple times, that is a completely different logic than before your change. It will not give dev warnings if NODE_ENV is not set, which was the case before.

if NODE_ENV is not set

typeof process === 'undefined' || process.env.NODE_ENV === 'production'

false || false

will give dev warnings, or you talking about non node environment without process variable?

[dimaMachina]

@phryneas Ok, I reread your suggestions, #3927 (comment) you can apply it
because it will be treeshaked rather than current approach https://github.com/graphql/graphql-js/blob/16.x.x/src/jsutils/instanceOf.ts?rgh-link-date=2023-06-26T09%3A34%3A10Z#L12

[phryneas]

will give dev warnings, or you talking about non node environment without process variable?

Anything like that.
The default behavior of this lib is "if nothing is specified by the user, output dev warnings".
Doesn't matter if it's process, process.env or process.env.NODE_ENV. (Although, admittedly, some of them would flat-out error, which is what the globalThis check tried to fix.)

If a user wanted to use this in ESM and disable dev warnings, they could still manually declare a global variable to turn them off.

In the end, I am not a maintainer of this lib and not the one you need to convince of a final change, but please be transparent what you are introducing here - and this one changes the current intent of the prod/dev check, disguised as just a bugfix.

[phryneas]

@phryneas Ok, I reread your suggestions, #3927 (comment) you can apply it because it will be treeshaked rather than current approach https://github.com/graphql/graphql-js/blob/16.x.x/src/jsutils/instanceOf.ts?rgh-link-date=2023-06-26T09%3A34%3A10Z#L12

Still leaves the problem that there is a bunch of bundlers out there that can never be configured to replace typeof anything statically (see my link from before).

What about globalThis.process !== 'undefined' && process.env.NODE_ENV === 'production'? That would still need configuration to perfectly tree-shake, but minimize any risk of string-replacement breakage, and the configuration should be possible in every environment.

Config for webpack would look like

config.plugins.push(
   new webpack.DefinePlugin({
     "globalThis.process": JSON.stringify({ process: { NODE_ENV: "production" } }),
   })
 );

For reference, I compiled a list of configurations for different bundlers over in this issue: apollographql/apollo-client#10915
Apollo Client will be going for globalThis.__DEV__ in the next version, which will need config pretty much everywhere, but cause much fewer collisions with incorrect replacements in Vite etc.

[dimaMachina]

there is a bunch of bundlers out there that can never be configured to replace

I don't know about bunch of, but webpack from Next.js replace fine typeof window and typeof process (it showed in my initial comment). Also, your link to the comment suggests defining process.browser but this is deprecated.

What about globalThis.process !== 'undefined' && process.env.NODE_ENV === 'production'? That would still need configuration

I guess you wanted to say globalThis.process !== undefined, I would suggest never using globalThis.process because no one bundler doesn't replace it out of the box.

[phryneas]

I don't know about bunch of, but webpack from Next.js replace fine typeof window and typeof process (it showed in my initial comment). Also, your link to the comment suggests defining process.browser but this is deprecated.

NextJs is moving away from Webpack, and Vite uses esbuild, so taking these other bundlers into account is definitely necesary.

I guess you wanted to say globalThis.process !== undefined, I would suggest never using globalThis.process because no one bundler doesn't replace it out of the box.

But they can be configured to replace it, while we are talking about the alternative of locking a big part of the ecosystem out of these optimizations completely. One extra line of bundler config won't kill you, if that means that all vite users get the chance to use the same optimization.

[dimaMachina]

@phryneas replaced to globalThis.process !== undefined && process.env.NODE_ENV === 'production'

[dimaMachina]

@phryneas you said

if that means that all vite users get the chance to use the same optimization.

but I saw your reply #3918 (comment) where vite users can replace typeof process with @rollup/plugin-replace https://github.com/rollup/plugins/tree/master/packages/replace#objectguards

[phryneas]

@B2o5T vite uses different bundlers for different environments, also esbuild is not only used by vite, it was just the first example that came to mind.

[eabp]

@phryneas Ok, I reread your suggestions, #3927 (comment) you can apply it because it will be treeshaked rather than current approach https://github.com/graphql/graphql-js/blob/16.x.x/src/jsutils/instanceOf.ts?rgh-link-date=2023-06-26T09%3A34%3A10Z#L12

Still leaves the problem that there is a bunch of bundlers out there that can never be configured to replace typeof anything statically (see my link from before).

What about globalThis.process !== 'undefined' && process.env.NODE_ENV === 'production'? That would still need configuration to perfectly tree-shake, but minimize any risk of string-replacement breakage, and the configuration should be possible in every environment.

Config for webpack would look like

config.plugins.push(
   new webpack.DefinePlugin({
     "globalThis.process": JSON.stringify({ process: { NODE_ENV: "production" } }),
   })
 );

For reference, I compiled a list of configurations for different bundlers over in this issue: apollographql/apollo-client#10915 Apollo Client will be going for globalThis.__DEV__ in the next version, which will need config pretty much everywhere, but cause much fewer collisions with incorrect replacements in Vite etc.

Following the track of this error I got it for Nextjs version 13.5.6, using Graphql version 16.8.0 and I was able to fix it using the solution proposed by @phryneas in the comment: #3927 (comment)

const nextConfig = {
  webpack(config, { webpack }) {
    config.plugins.push(
      new webpack.DefinePlugin({
        "globalThis.process": JSON.stringify({
          env: {
            NODE_ENV: "production",
          },
        }),
      })
    );
    return config;
  },
};

module.exports = nextConfig;

[JoviDeCroock]

Fixed with #4022