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

Open
srmagura opened this issue Jun 21, 2023 · 16 comments · May be fixed by #3927
Open

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

srmagura opened this issue Jun 21, 2023 · 16 comments · May be fixed by #3927

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/app-vite@1.4.3
│ ├─┬ @quasar/vite-plugin@1.4.0
│ │ ├── @vitejs/plugin-vue@2.3.4 deduped
│ │ ├── vite@2.9.16 deduped

ETA:

├── graphql@16.7.1

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 graphql@16.7.1

@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

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.

16 participants