avoid using `process.env` with `globalThis` #3927

dimaMachina · 2023-06-24T00:08:34Z

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 · 2023-06-24T00:08:40Z

✅ 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 · 2023-06-24T00:10:05Z

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

src/jsutils/instanceOf.ts

phryneas · 2023-06-26T06:18:34Z

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 · 2023-06-26T08:56:24Z

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 · 2023-06-26T09:12:58Z

@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 · 2023-06-26T09:25:47Z

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 · 2023-06-26T09:29:20Z

@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 · 2023-06-26T09:29:33Z

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 · 2023-06-26T09:33:19Z

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 · 2023-06-26T09:33:20Z

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 · 2023-06-26T09:34:10Z

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 · 2023-06-26T09:44:21Z

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 · 2023-06-26T09:48:44Z

@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 · 2023-06-26T09:49:43Z

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.

Co-authored-by: Lenz Weber-Tronic <mail@lenzw.de>

phryneas · 2023-06-26T09:55:32Z

@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 · 2023-06-26T16:53:18Z

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 · 2023-06-26T17:14:02Z

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 · 2023-06-27T12:43:08Z

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

dimaMachina · 2023-06-27T12:51:14Z

@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 · 2023-06-27T17:01:16Z

@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 · 2023-10-19T16:31:26Z

@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;

avoid using globalThis with process

6cc7d06

oidualc mentioned this pull request Jun 24, 2023

Error in the browser console when -> import { setupWorker } from 'msw' mswjs/msw#1645

Closed

4 tasks

add typeof process === 'undefined' to make everybody happy

e0f522a

dotansimha approved these changes Jun 25, 2023

View reviewed changes

dotansimha requested a review from IvanGoncharov June 25, 2023 06:33

phryneas reviewed Jun 26, 2023

View reviewed changes

src/jsutils/instanceOf.ts Outdated Show resolved Hide resolved

Update src/jsutils/instanceOf.ts

ef2405f

Co-authored-by: Lenz Weber-Tronic <mail@lenzw.de>

fix review

06e2d8a

dimaMachina changed the title ~~avoid using globalThis with process~~ avoid using process.env with globalThis Jun 27, 2023

Aietes mentioned this pull request Jun 29, 2023

rollup-plugin-inject: failed to parse graphql/jsutils/instanceOf.mjs #3928

Open

This was referenced Jun 29, 2023

Nuxt cloudflare-pages deployment fails nuxt/nuxt#21768

Closed

Nuxt cloudflare-pages deployment fails unjs/nitro#1346

Closed

calvincestari mentioned this pull request Dec 4, 2023

chose(deps): update GraphQLCompiler dependencies apollographql/apollo-ios-dev#181

Merged

This was referenced Feb 14, 2024

fix#3978 #4021

Open

fix: remove globalThis check and align with what bundlers can accept #4022

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

avoid using `process.env` with `globalThis` #3927

avoid using `process.env` with `globalThis` #3927

dimaMachina commented Jun 24, 2023 •

edited

netlify bot commented Jun 24, 2023 •

edited

github-actions bot commented Jun 24, 2023

phryneas commented Jun 26, 2023 •

edited

dimaMachina commented Jun 26, 2023 •

edited

phryneas commented Jun 26, 2023 •

edited

dimaMachina commented Jun 26, 2023

dimaMachina commented Jun 26, 2023

phryneas commented Jun 26, 2023

dimaMachina commented Jun 26, 2023

phryneas commented Jun 26, 2023

phryneas commented Jun 26, 2023 •

edited

dimaMachina commented Jun 26, 2023

dimaMachina commented Jun 26, 2023

phryneas commented Jun 26, 2023

phryneas commented Jun 26, 2023 •

edited

dimaMachina commented Jun 26, 2023

phryneas commented Jun 26, 2023

dimaMachina commented Jun 27, 2023

dimaMachina commented Jun 27, 2023

phryneas commented Jun 27, 2023

eabp commented Oct 19, 2023 •

edited

avoid using process.env with globalThis #3927

Are you sure you want to change the base?

avoid using process.env with globalThis #3927

Conversation

dimaMachina commented Jun 24, 2023 • edited

Input

Output (by webpack in Next.js)

Conclusion

netlify bot commented Jun 24, 2023 • edited

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

github-actions bot commented Jun 24, 2023

phryneas commented Jun 26, 2023 • edited

dimaMachina commented Jun 26, 2023 • edited

phryneas commented Jun 26, 2023 • edited

dimaMachina commented Jun 26, 2023

dimaMachina commented Jun 26, 2023

phryneas commented Jun 26, 2023

dimaMachina commented Jun 26, 2023

phryneas commented Jun 26, 2023

phryneas commented Jun 26, 2023 • edited

dimaMachina commented Jun 26, 2023

dimaMachina commented Jun 26, 2023

phryneas commented Jun 26, 2023

phryneas commented Jun 26, 2023 • edited

dimaMachina commented Jun 26, 2023

phryneas commented Jun 26, 2023

dimaMachina commented Jun 27, 2023

dimaMachina commented Jun 27, 2023

phryneas commented Jun 27, 2023

eabp commented Oct 19, 2023 • edited

avoid using `process.env` with `globalThis` #3927

avoid using `process.env` with `globalThis` #3927

dimaMachina commented Jun 24, 2023 •

edited

netlify bot commented Jun 24, 2023 •

edited

phryneas commented Jun 26, 2023 •

edited

dimaMachina commented Jun 26, 2023 •

edited

phryneas commented Jun 26, 2023 •

edited

phryneas commented Jun 26, 2023 •

edited

phryneas commented Jun 26, 2023 •

edited

eabp commented Oct 19, 2023 •

edited