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

rollup-plugin-inject: failed to parse graphql/jsutils/instanceOf.mjs #3928

Closed
iBobik opened this issue Jun 27, 2023 · 8 comments
Closed

rollup-plugin-inject: failed to parse graphql/jsutils/instanceOf.mjs #3928

iBobik opened this issue Jun 27, 2023 · 8 comments

Comments

@iBobik
Copy link

iBobik commented Jun 27, 2023

When this library is used in the Nuxt 3 project and compiled for Cloudflare it fails like this:

ℹ Building Nitro Server (preset: cloudflare-pages)                  nitro 19:16:00
(inject plugin) rollup-plugin-inject: failed to parse /home/projects/github-twqhau-upewai/node_modules/graphql/jsutils/instanceOf.mjs. Consider restricting the plugin to particular files via options.include

[nitro 19:16:14]  ERROR  RollupError: Unexpected token (Note that you need plugins to import files that are not JavaScript)


10:   /* c8 ignore next 6 */
11:   // FIXME: https://github.com/graphql/graphql-js/issues/2317
12:   globalThis.process && globalThis.process.env.NODE_ENV === 'production'
                                       ^
13:     ? function instanceOf(value, constructor) {
14:         return value instanceof constructor;


[19:16:14]  ERROR  Unexpected token (Note that you need plugins to import files that are not JavaScript)

  at error (<anonymous>)
  at setSource (<anonymous>)
  at  (<anonymous>)

Happens in version 16.7.1 and 16.7.0 but works with 16.6.0 and 17.0.0-alpha.2.

Repro: https://stackblitz.com/edit/github-twqhau-upewai
Start with: NITRO_PRESET=cloudflare-pages npm run build

@Aietes
Copy link

Aietes commented Jun 29, 2023

This is also tracked as an issue in Nuxt and nitro: nuxt/nuxt#21768. Other issues like #3918 also refer to the same problem, but it seem it will be solved soon by PR #3927.

The module tries to access globalThis.process, which makes the Cloudflare pages deployment fail. You can't access process in Cloudflare, as it's a node-only API. Technically it should be poyfilled by unenv, but there seems to be an issue preventing this. As a work-around, in our project we are currently patching the respective code in the graphql module.

pnpm patch graphql
code <generated_folder>

In node_modules/graphql/jsutils/instanceOf.mjs remove the code that tries to access process.

export const instanceOf = function instanceOf(value, constructor) {
  return value instanceof constructor
}

Apply the patch.

pnpm patch-commit <generated_folder>

The resulting patch should appear in package.json to be applied during deployment on Cloudflare.

  "pnpm": {
    "patchedDependencies": {
      "[email protected]": "patches/[email protected]"
    }
  }

@utsavkapoor
Copy link

Any updates on the fix ? I was checking the PR but that also doesnt seem to have merged.

@jgalbsss
Copy link

Bumping this...

@websmithcode
Copy link

UP

1 similar comment
@sofialapteva
Copy link

UP

@luisfrocha
Copy link

It appears this is still not fixed. I just ran into it the last few days. Any word on when it'll get fixed?

@scottopherson
Copy link

#4022 seems close to ready.

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

No branches or pull requests

10 participants