-
Notifications
You must be signed in to change notification settings - Fork 2k
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
check "globalThis.process" before accessing it #3887
Conversation
kettanaito
commented
Apr 24, 2023
- Backports c51c895 from fix: only access Node.js globals if available #3501 (I tried cherry-picking but the object was inaccessible, I simply moved the changes)
- Closes process is not defined in browsers #3758
|
Hi @kettanaito, I'm @github-actions bot happy to help you with this PR 👋 Supported commandsPlease post this commands in separate comments and only one per comment:
|
src/jsutils/instanceOf.ts
Outdated
@@ -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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done ✅
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. |
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! |
@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? :) |
I can provide any additional information. just let me know. thanks |
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.