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

Regression: Importing the package fails in a brand new Vite project #3918

Closed
srmagura opened this issue Jun 21, 2023 · 19 comments
Closed

Regression: Importing the package fails in a brand new Vite project #3918

srmagura opened this issue Jun 21, 2023 · 19 comments

Comments

@srmagura
Copy link

Minimal repro: https://github.com/srmagura/graphql-repro

The project was created with:

npm create-vite@latest graphql-repro

Then I chose "Vanilla" and "TypeScript" at the prompts.

Install the graphql npm package and add the import

import * as graphql from "graphql";

It produces an error like:

Uncaught SyntaxError: Unexpected string (at graphql.js?v=4b27e75c:1248:113)

If you click on the error in the console, you'll see that process.env.NODE_ENV has been replaced with "development".

image

Let me know if you think this is a Vite issue instead, and I will report it there.

@airhorns
Copy link

airhorns commented Jun 21, 2023

This got us too -- I think the regexp here is not working quite right: https://github.com/vitejs/vite/blob/main/packages/vite/src/node/plugins/clientInjections.ts#L88-L95 . I think this is a bonified vite bug but because both libraries are so popular this error is going to cause a lot of pain. Worth yanking, or changing the transpilation to work fine with vite out of the box?

@bburr
Copy link

bburr commented Jun 21, 2023

Just adding more info - the $ in _globalThis$process seen here is what's not accounted for in the regexp - https://diff.intrinsic.com/graphql/16.6.0/16.7.0#file-jsutils__instanceOf.mjs

So _globalThis$process.env.NODE_ENV gets changed to _globalThis$"development" by Vite

@IvanGoncharov
Copy link
Member

@srmagura It's an issue with Vite internals, but I added a workaround for it in 16.7.1
Can you please confirm that it is fixed?

@KangMiSun17
Copy link

@IvanGoncharov I was having the same problem and it was solved. Thank you.

@michaelLoeffelmann
Copy link

i still have the problem, using the 16.7.1 and vite 2.9.16.
when i undo your fix, it's working.

@lemonmade
Copy link

@IvanGoncharov I believe that the issue is still present, even with your change applied:

Screenshot 2023-06-23 at 2 46 14 PM

I believe a more common way of checking whether you are in Node is to do typeof process === 'object', like this: https://github.com/graphql/graphql-js/compare/16.x.x...lemonmade:graphql-js:fix-process-env-replacement?expand=1. @rollup/plugin-replace, which is also affected by this issue, actually has special handling for this code pattern: https://github.com/rollup/plugins/tree/master/packages/replace#objectguards.

@dimaMachina
Copy link

Why this happens I wrote in #3925

@phryneas
Copy link

phryneas commented Jun 26, 2023

@lemonmade did you change the default configuration of @rollup/plugin-replace?

According to the documentation on the delimiters option, the default value for that option should guard against deep property access.

Or are you using another bundler and only also mention that rollup plugin?

typeof process is problematic, as some other bundlers that actually look at an AST and don't just do blind string replacement (e.g. esbuild) are not able to replace that at all, as it is a full statement, not just a variable name/access.

@lemonmade
Copy link

@phryneas thanks for the response. I checked again and it appears that this issue only affects the development build of Vite, and only up until version 4.2.0. Here's an example showing a slightly older Vite still getting bit by this bug. I can definitely update Vite to fix this for myself, though!

On typeof process — I think bundlers like ESBuild could still replace process with the literal undefined, couldn't they? I imagine that might not be a safe operation since it could conflict with bindings users create themselves, though.

@salcedo
Copy link

salcedo commented Jun 27, 2023

Error still present on Quasar Framework with Vite

 » Dev mode............... spa
 » Pkg quasar............. v2.12.0
 » Pkg @quasar/app-vite... v1.4.3
├─┬ @quasar/[email protected]
│ ├─┬ @quasar/[email protected]
│ │ ├── @vitejs/[email protected] deduped
│ │ ├── [email protected] deduped

ETA:

Rolling back to 16.6.0 for now but don't know how long that's gonna fly.

@NGPixel
Copy link

NGPixel commented Jul 1, 2023

Same problem here with Quasar + Vite with [email protected]

@davidsims9t
Copy link

I tried rolling back to 16.6.0 and I'm still having the same issue with Vite.

@phryneas
Copy link

phryneas commented Jul 1, 2023

@davidsims9t in that case, maybe clear your vite cache?

@jptaranto
Copy link

Hitting this using Rollup as well as we're replacing the NODE_ENV key for React production builds as per this ancient technique: rollup/rollup#208

The fix for me in this case was to add another replace option for graphql in rollup.config.js:

replace({
  "globalThis.process.env.NODE_ENV": JSON.stringify(process.env.NODE_ENV),
  "process.env.NODE_ENV": JSON.stringify(process.env.NODE_ENV),
  preventAssignment: true,
}),

@slhmy
Copy link

slhmy commented Sep 5, 2023

I tried rolling back to 16.6.0 and I'm still having the same issue with Vite.

16.5.0 will be fine in my case.

@romanpastu
Copy link

I had to downgrade all the way down to 15.6.1 for it to work, for me it failed in graphql 16.6.0 with vite , suddenly out of nowhere while i was developing. After that I have not been bale to do anything to restore it, and I ended up having to test previous version, settled at 15.6.1

@unckleg
Copy link

unckleg commented May 23, 2024

Any updates?

@JoviDeCroock
Copy link
Member

JoviDeCroock commented May 23, 2024

I hope to be merging #4022 after the next WG which should remove these issues... We have investigated more in #4075

A workaround would be to add globalThis.process and globalThis.process.env.NODE_ENV to your define config.

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
@JoviDeCroock
Copy link
Member

This is fixed in #4022 and published in GraphQL 16.8.2, you can find the instructions for various bundlers here on how to remove it in production.

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.