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

Error after upgrading from 16.6 to 16.8.1 with globalThis.process.env.NODE_ENV #3978

Closed
DontRepeatYourself opened this issue Oct 12, 2023 · 13 comments

Comments

@DontRepeatYourself
Copy link

DontRepeatYourself commented Oct 12, 2023

Running a project with Vue3 / Quasar2 / Vite in development mode fails after update.

// node_modules/graphql/jsutils/instanceOf.mjs
globalThis.process && globalThis.process.env.NODE_ENV === 'production'

is compiled to

var instanceOf = globalThis.process && globalThis."development" === "production" ? function instanceOf2(value, constructor) { ... }

Changing

globalThis.process && globalThis.process.env.NODE_ENV === 'production'

to

globalThis.process && globalThis.process.env['NODE_ENV'] === 'production'

solves the problem for me.

@vdineva
Copy link

vdineva commented Oct 12, 2023

Experiencing the same issue and this bug does not allow us to upgrade to the latest version and there's a security advisory for versions before 16.8.1: GHSA-9pv7-vfvm-6vr7

@rafaelsorto
Copy link

Having the same bug. It resolves to globalThis."development" === 'production' or production and it breaks the app.

@MaLiN2223
Copy link

MaLiN2223 commented Oct 28, 2023

Got the same issue when using the ReactJS framework, keeping graphql on 16.6.0 seems to be the solution for us for now.

@TdyP
Copy link

TdyP commented Dec 4, 2023

Same issue here, with a Uncaught SyntaxError: missing name after . operator at runtime.
React project compiled with Vite.

Any plans for a fix?

Thanks

arthuravianna added a commit to prototyp3-dev/cartesi-client that referenced this issue Dec 18, 2023
The newest versions of the package presented a bug as for this issue graphql/graphql-js#3978.
@vdineva
Copy link

vdineva commented Jan 4, 2024

Just checking on the status of this issue. Is there a plan to address it and publish a new version? It's been open for a while now

@vdineva
Copy link

vdineva commented Feb 12, 2024

For anyone experiencing issues with v16.8.1, a workaround is to define globalThis.process.env.NODE_ENV in your bundler.
For Webpack it's:

new DefinePlugin({
   "globalThis.process.env.NODE_ENV": JSON.stringify(process.env.NODE_ENV)
})

This was referenced Feb 13, 2024
@mobsean
Copy link

mobsean commented Feb 13, 2024

see PR, this fixes the vue3, quasar, vite problem with graphql-js > 16.6

@JoviDeCroock
Copy link
Member

Hey,

Would anyone mind posting a reproduction to this issue leveraging vite/webpack/.... Does upgrading resolve the issue?

@mobsean
Copy link

mobsean commented Feb 15, 2024

Hey,

Would anyone mind posting a reproduction to this issue leveraging vite/webpack/.... Does upgrading resolve the issue?

I checked your PR and fine with it. The problem in this issue is solved by this.

@n05la3
Copy link

n05la3 commented Feb 15, 2024

Hey,

Would anyone mind posting a reproduction to this issue leveraging vite/webpack/.... Does upgrading resolve the issue?

Tested with vite and it works fine.

@JoviDeCroock
Copy link
Member

What did you test @n05la3 the PR or whether the issue is present 😅 we need versions/... actual reproductions or at the very least version numbers of where this issue shows itself

@n05la3
Copy link

n05la3 commented Feb 15, 2024

I misread the comment. I experienced this issue in a Quasar 2.14.3 project with Vite 5.0.0 when upgrading from 16.6.0 to 16.8.1. I had used a patch to bypass this issue. Earlier today, I tested the changes in the PR in my project and it works fine.

saihaj pushed a commit that referenced this issue 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
@DontRepeatYourself
Copy link
Author

Fixed with 16.8.2

yaacovCR pushed a commit to yaacovCR/graphql-js that referenced this issue Aug 19, 2024
graphql#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 graphql#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 graphql#3978
Fixes graphql#3918
Fixes graphql#3928
Fixes graphql#3758
Fixes graphql#3934

This purposefully does not account for
graphql#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
graphql#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 graphql#4021
Supersedes graphql#4019
Supersedes graphql#3927

> This now also adds a documentation page on how to remove all of these
yaacovCR pushed a commit to yaacovCR/graphql-js that referenced this issue Aug 19, 2024
graphql#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 graphql#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 graphql#3978
Fixes graphql#3918
Fixes graphql#3928
Fixes graphql#3758
Fixes graphql#3934

This purposefully does not account for
graphql#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
graphql#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 graphql#4021
Supersedes graphql#4019
Supersedes graphql#3927

> This now also adds a documentation page on how to remove all of these
yaacovCR pushed a commit that referenced this issue Sep 6, 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 a pull request may close this issue.

8 participants