Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Optional Chaining to globalThis.process.env.NODE_ENV Retrieval #4019

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

johnfabre
Copy link

This PR introduces optional chaining when accessing globalThis.process.env.NODE_ENV in the graphql-js codebase.

The current implementation assumes that globalThis.process.env is always defined, which may not be the case in all environments. This assumption leads to a runtime error when NODE_ENV is accessed on an undefined process.env.

By adding optional chaining (?.), we ensure that NODE_ENV is only accessed if globalThis.process and process.env are defined, thereby preventing potential runtime errors.

This change improves the robustness of the code and makes it more compatible with various JavaScript environments.

Copy link

linux-foundation-easycla bot commented Feb 12, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link

netlify bot commented Feb 12, 2024

Deploy Preview for compassionate-pike-271cb3 ready!

Name Link
🔨 Latest commit dd99b7e
🔍 Latest deploy log https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/65c9e51f1faac900081b9445
😎 Deploy Preview https://deploy-preview-4019--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@JoviDeCroock
Copy link

This is a duplicate of #4017

this will fix the issue where globalThis.process.env is undefined and it is still trying to retrieve NODE_ENV
@johnfabre
Copy link
Author

johnfabre commented Feb 12, 2024

CLA SignedThe committers listed above are authorized under a signed CLA.

added myself to CLA

This is a duplicate of #4017

used the code suggested in here
https://github.com/graphql/graphql-js/pull/4017/files#r1483265402

saihaj pushed a commit that referenced this pull request May 29, 2024
#4022)

As surfaced in
[Discord](https://discord.com/channels/625400653321076807/862957336082645006/1206980831915282532)
this currently is a breaking change in the 16.x.x release line which is
preventing folks from upgrading towards a security fix. This PR should
result in a patch release on the 16 release line.

This change was originally introduced to support CFW and browser
environments which should still be supported with the `typeof` check CC
@n1ru4l

This also adds a check whether `.env` is present as in the DOM using
`id="process"` defines that as a global which we don't want to access on
accident. as shown in #4017

Bundles also target `process.env.NODE_ENV` specifically which fails when
it replaces `globalThis.process.env.NODE_ENV` as this becomes
`globalThis."production"` which is invalid syntax.

Fixes #3978
Fixes #3918
Fixes #3928
Fixes #3758
Fixes #3934

This purposefully does not account for
#3925 as we can't address
this without breaking CF/plain browsers so the small byte-size increase
will be expected for bundled browser environments. As a middle ground we
did optimise the performance here. We can revisit this for v17.

Most bundlers will be able to tree-shake this with a little help, in
#4075 (comment)
you can find a conclusion with a repo where we discuss a few.

- Next.JS by default replaces
[`process.env.NODE_ENV`](https://github.com/vercel/next.js/blob/b0ab0fe85fe8c93792051b058e060724ff373cc2/packages/next/webpack.config.js#L182)
you can add `typeof process` linearly
- Vite allows you to specify
[`config.define`](https://vitejs.dev/config/shared-options.html#define)
- ESBuild by default will replace `process.env.NODE_ENV` but does not
support replacing `typeof process`
- Rollup has a plugin for this
https://www.npmjs.com/package/@rollup/plugin-replace

Supersedes #4021
Supersedes #4019
Supersedes #3927

> This now also adds a documentation page on how to remove all of these
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants