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

process is not defined in browsers #3758

Closed
justinfagnani opened this issue Oct 15, 2022 · 9 comments
Closed

process is not defined in browsers #3758

justinfagnani opened this issue Oct 15, 2022 · 9 comments

Comments

@justinfagnani
Copy link

justinfagnani commented Oct 15, 2022

Even though this seems to have been at least partially addressed in #1058 there are still references to process in the code, like https://github.com/graphql/graphql-js/blob/main/src/jsutils/instanceOf.ts#L12 that cause errors when loading modules in the browser.

@yaacovCR
Copy link
Contributor

yaacovCR commented Oct 15, 2022

Can you include more details about the error you are experiencing? The ? operator in the line you referenced is supposed to help avoid an error if process is undefined...

@kettanaito
Copy link
Contributor

kettanaito commented Apr 20, 2023

This issue is still reproducible on the latest version graphql@16.6.0 despite the fix being merged a year ago (#3501). It does seem to exist in the latest HEAD, so I assume it hasn't been released yet?

@yaacovCR, I can provide more details about the issue.

Details

What happens?

The graphql package fails to run in the browser.

How do you know that's the case?

Because it throws the following error on runtime:

ReferenceError: process is not defined
  at /node_modules/.pnpm/graphql@16.6.0/node_modules/graphql/jsutils/instanceOf.mjs:13:3

This is a rightful error since the latest published version relies on the global process that doesn't exist in the browser.

How to reproduce this?

npm init -y
npm install graphql@16.6.0 # latest at the moment
<!-- index.html -->
<script type="module">
  import graphql from 'graphql'
</script>
// wds.js
import { startDevServer } from '@web/dev-server'

const wds = await startDevServer({
 config: {
    nodeResolve: {
      exportConditions: ['browser'],
    },
  },
})

The ESM setup may be irrelevant, I'm just sharing how I'm reproducing it right now.

node ./wds.js

Open the served index.html in the browser, open the browser's DevTools, go to "Console", see the mentioned exception.

How can this be resolved?

This package shouldn't rely on Node.js globals if it's intended to run in the browser also. Based on the existence of the #3501 and #1058, I assume that there is such an intention.

The action here may be to release the 16.x version that contained the #3501 fix so you don't have to go full major bump that's currently in alpha tag.

Let me know if I can help. I would love to see this fixed.

@Andarist
Copy link

It's worth noting here that nowadays package.json#exports are a perfect replacement for the process.env.NODE_ENV !== 'production' convention (at the expense of complicating the build process)

@abooayoob
Copy link

@Andarist Can I ask for any pointers on what this means? package.json#exports are a perfect replacement for the process.env.NODE_ENV !== 'production'

@justinfagnani
Copy link
Author

If means that your default export condition can be the production build and the 'development' export condition can be the development build.

@yaacovCR
Copy link
Contributor

The action here may be to release the 16.x version that contained the #3501 fix so you don't have to go full major bump that's currently in alpha tag.

Let me know if I can help. I would love to see this fixed.

Backporting #3501 to the 16.x branch seems like a great idea. A PR would definitely be appreciated and then we can ping @IvanGoncharov for a release. Thanks!

@kettanaito
Copy link
Contributor

@yaacovCR, will open the PR then against the latest 16.x state of the repo. Will let you know.

@kettanaito
Copy link
Contributor

@yaacovCR, the pull request with the backport for 16.x is open at #3887. cc @IvanGoncharov, if I may, I would appreciate a quick look at that fix to unblock MSW and other consumers experiencing this issue. Thanks!

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

6 participants