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

Syntax error in Nuxt project due to graphql-js pull request #3887 #3919

Closed
okaufmann opened this issue Jun 21, 2023 · 20 comments
Closed

Syntax error in Nuxt project due to graphql-js pull request #3887 #3919

okaufmann opened this issue Jun 21, 2023 · 20 comments

Comments

@okaufmann
Copy link

Hi

It looks like #3887 is causing problems in my Nuxt project. When compiled, there is a syntax error which breaks the app completely:
CleanShot 2023-06-22 at 00 18 38

((_globalThis$process = globalThis.process) === null || _globalThis$process === void 0 ? void 0 : _globalThis$"development") === "production" ? function instanceOf2(value, constructor) {
      return value instanceof constructor;
    }

v16.6.0 works perfectly.

I cannot verify if this is only a problem when used with Nuxt or not.


@tnyo43
Copy link

tnyo43 commented Jun 22, 2023

I ran into the same problem with a React project.

@thepassle
Copy link

It looks like the code published by graphql is fine: https://unpkg.com/browse/[email protected]/jsutils/instanceOf.mjs

image

And can be imported as is without any problems:

image

So it almost feels like a bug in whatever is transpiling your project, which (judging from your screenshot) also looks like esbuild output. However, running the same code through esbuild again, doesn't result in the error for me:
image

Do you know which esbuild configuration options your build process uses? And are there are tools that may transpile/compile your code?

@Josef-Reichardt
Copy link

I ran into the same issue in a vite / vue / ts project after upgarding from 16.6.0 to 16.7.0. Unfortunately I'm not able to reproduce the issue in a fresh project. So I'm not sure which combination of libs or tool is the reason for this error.

I'm using node v18.14.0, yarn 1.22.19, vite 4.3.9, vue 3.3.4, typescript 5.1.3, vue-tsc 1.8.1, @esbuild-plugins/node-globals-polyfill 0.2.3, @esbuild-plugins/node-modules-polyfill 0.2.2, and many more ...

The issue only appears if I run the application in dev mode. The production build works fine. 🤔

@mpgalaxy
Copy link

It looks like the code published by graphql is fine: https://unpkg.com/browse/[email protected]/jsutils/instanceOf.mjs

image And can be imported as is without any problems: image So it almost feels like a bug in whatever is transpiling your project, which (judging from your screenshot) also looks like esbuild output. However, running the same code through esbuild again, doesn't result in the error for me: image

Do you know which esbuild configuration options your build process uses? And are there are tools that may transpile/compile your code?

Are you sure the code is fine ? Shouldn't it be

: _globalThis$(process.env.NODE_ENV) === 'production'

? ;)

@thepassle
Copy link

Yeah, I'm sure the code is fine. Why do you think it should be something else?

@Vincz
Copy link

Vincz commented Jun 22, 2023

Same here. The generated code is invalid, a ( is missing.

@razorness
Copy link

razorness commented Jun 22, 2023

Hey, I am using ViteJS and facing same problem. It looks like ViteJS/esbuild is replacing process.env.NODE_ENV on compile time. Which results in _globalThis$"development") === "production".

I am questioning if you are forced to use globalThis at this point.

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

The compiled output of ViteJS/esbuild should look like this: "development" === "production" in development environment and "production" === "production" in production environment.

@thepassle
Copy link

Same here. The generated code is invalid, a ( is missing.

Aha, I see what you're saying. That makes sense indeed. I imported the module in a browser to check for syntax errors, and when I saw none I assumed that the code was correct, but didnt actually verify the logic.

That would mean the issue is likely somewhere in the buildtooling that graphql uses internally. I try to see if I can figure out where exactly this is going wrong.

@thepassle
Copy link

I cloned the main branch, ran npm install and ran a npm run build:npm, but I don't seem able to reproduce it. The output looks as follows:
image

But the output as published on npm has some transpiled code in it:
image

So I'm unable to reproduce it locally. I also checked the ci files to see if maybe any transpilation is happening before publishing, but that also doesnt seem to be the case.

I also noticed that the tsconfig options were recently changed from es2020 to es2021 and es2022, so I tried running the build with version es2020 to see if that caused the issue, but that also didnt seem to be the case.

So its not clear to me where the transpilation of optional chaining is taking place. Maybe a maintainer (or anybody else) can also try to verify my findings by locally installing and running a build, and checking npmDist/jsutils/instanceOf.mjs?

@razorness
Copy link

@thepassle if you replace

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

against process.env.NODE_ENV === 'production', the npm build looks the same as of v16.6.0:

image

@thepassle
Copy link

thepassle commented Jun 22, 2023

Indeed :) but then the module will still not be able to run in a browser (which is the case for MSW), because the process global doesnt exist there

And if you leave globalThis?.env.NODE_ENV === 'production', the output is (as expected):
image

IvanGoncharov added a commit that referenced this issue 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.
@IvanGoncharov
Copy link
Member

IvanGoncharov commented Jun 22, 2023

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

@razorness
Copy link

It works now with ViteJS v4 (esbuild). But: since you always expect globalThis.process to be existend, isn't it the case that the code never reaches globalThis.process.env.NODE_ENV === 'production' in browser?

@IvanGoncharov
Copy link
Member

It works now with ViteJS v4 (esbuild). But: since you always expect globalThis.process to be existend, isn't it the case that the code never reaches globalThis.process.env.NODE_ENV === 'production' in browser?

@razorness For that, you need to define NODE_ENV to production (done by default in most of bundlers).

@okaufmann
Copy link
Author

@okaufmann It's an issue with Nuxt internals, but I added a workaround for it in 16.7.1

Can you please confirm that it is fixed?

@IvanGoncharov Thanks for the quick fix it does work perfectly again 🎉

@WulfP
Copy link

WulfP commented Jul 13, 2023

Running into the same issue with quasar 2.12.2 / vite.

Not sure if it is an issue if bundlers replace process.env.NODE_ENV.
I would expect the bundlers to do this.

I created a very simple workarround:

Just change

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

to

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

and everything works fine because nothing is replaced unexpectedly.

For an instant fix you can run

sed "s/process\.env\.NODE_ENV/process.env['NODE_ENV']/g" ./instanceOf.mjs

manually or use this script

#!/bin/bash
fileBase='./node_modules/graphql/jsutils/instanceOf'
mv "${fileBase}.mjs" "${fileBase}.tmp"
sed "s/process.env.NODE_ENV/process.env['NODE_ENV']/g" "${fileBase}.tmp" > "${fileBase}.mjs"
rm "${fileBase}.tmp"

in your build-pipline.

Don´t forget to delete the build-cache after patching the file, in my case rm -rf node_modules/.q-cache/ does the job.

@hipertracker
Copy link

Hacking node_module? It's not the best idea.

@hipertracker
Copy link

The problem hits only Quasar. :( Vuertify3 which uses also Vite has no problem with that.
Quasar fails miserably on that line:

import gql from "graphql-tag";

@WulfP
Copy link

WulfP commented Aug 29, 2023

Hacking node_module? It's not the best idea.

Imo this is a patch, not a hack. And perfectly fine to handle smaller bugs like this.
But I notice that we have very different opinions.

Maybe quasar does not work like you would expect. Ok.
But I do not think that it "fails miserably".

In my current project, I replaced Nuxt / Vuetify / Apollo, a horrible combination by quasar / graphql-codegen.
I´m very happy with this decision. Better documentation, better component integration (imo Vuetify is miserable there),
no problems (except the reported one).

Yes, consider quasar as the answer to Nuxt / Vuetify but you are free to stick on your well-known solutions.

@asantTMC
Copy link

Experiencing the same issue in a React 18 + Vite 4.1 project. Updating Vite to >=4.2 solves the issue regardless of the graphql version

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

No branches or pull requests