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

check "globalThis.process" before accessing it #3887

Merged
merged 2 commits into from May 2, 2023

Conversation

kettanaito
Copy link

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 24, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: kettanaito / name: Artem Zakharchenko (9c6b6da)

@github-actions
Copy link

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

@@ -10,7 +10,7 @@ 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
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like ci thinks this line can be deleted

Copy link
Author

Choose a reason for hiding this comment

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

Will remove it and push the changes. Thanks for reviewing this, @yaacovCR!

Copy link
Author

Choose a reason for hiding this comment

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

Done ✅

@kettanaito
Copy link
Author

I've removed the eslint-disable line the Linter was complaining about. The CI should be passing now (would appreciate it if someone triggered it). If these changes look fine otherwise, please, let's release them as a backport as 16.x.

@IvanGoncharov IvanGoncharov merged commit 84bb146 into graphql:16.x.x May 2, 2023
17 checks passed
@kettanaito kettanaito deleted the fix/nodejs-process-access branch May 5, 2023 10:57
@kettanaito
Copy link
Author

Hi, @IvanGoncharov 👋 Sorry for tagging you directly but could you please get this released as 16.6.1 (I assume)? We really need these changes to unblock the next MSW release, I would highly appreciate your help!

@thepassle
Copy link

thepassle commented Jun 12, 2023

@yaacovCR @IvanGoncharov Is there anything we can do to help this get released? The code change is very minimal, and the PR itself has already been merged, what can we do to pull this over the finish line and get it published? :)

@IvanGoncharov IvanGoncharov added PR: bug fix 🐞 requires increase of "patch" version number PR: feature 🚀 requires increase of "minor" version number and removed PR: bug fix 🐞 requires increase of "patch" version number labels Jun 21, 2023
@websitevirtuoso
Copy link

I have vue project. after this fix I am starting getting error
image

@websitevirtuoso
Copy link

I can provide any additional information. just let me know. thanks

IvanGoncharov added a commit that referenced this pull request Jun 22, 2023
Fixes: #3919 #3920 #3921
Context: #3887 changed code and introced optinal chaining.
`globalThis.process?.env.NODE_ENV` is transpiled into
```
(_globalThis$process = globalThis.process) === null ||
_globalThis$process === void 0
  ? void 0
  : _globalThis$process.env.NODE_ENV;
```
Bundlers incorrectly replace (probably RegExp) `process.env.NODE_ENV` with `"development"` resulting in:
```
(_globalThis$process = globalThis.process) === null ||
_globalThis$process === void 0
  ? void 0
  : _globalThis$"development";
```

Technically it's not a graphql issue but an issue with bundler but since it caused so much pain in comutinity this is an attempt to fix it within our codebase.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature 🚀 requires increase of "minor" version number
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants