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

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

Merged
merged 8 commits into from
May 29, 2024

Conversation

JoviDeCroock
Copy link
Member

@JoviDeCroock JoviDeCroock commented Feb 14, 2024

As surfaced in Discord 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.

Supersedes #4021
Supersedes #4019
Supersedes #3927

This now also adds a documentation page on how to remove all of these

Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

Hi @JoviDeCroock, 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

@JoviDeCroock JoviDeCroock force-pushed the fix-node-env-issue branch 3 times, most recently from a8f5b4a to b0ca7e0 Compare February 14, 2024 14:16
@JoviDeCroock
Copy link
Member Author

CC @graphql/graphql-js-reviewers

@saihaj saihaj added the PR: bug fix 🐞 requires increase of "patch" version number label May 29, 2024
@JoviDeCroock
Copy link
Member Author

CI is failing with a rate limit issue, I can't force a retry 😅

@saihaj saihaj merged commit 0d12b06 into graphql:16.x.x May 29, 2024
14 of 15 checks passed
@benjie
Copy link
Member

benjie commented Jun 4, 2024

Love love love the documentation page; excellent work! 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: bug fix 🐞 requires increase of "patch" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants