Prevents injecting Node.js globals like process, which can break browser features when loading graphql from ESM-based CDN like esm.sh

[dimaMachina]

see https://esm.sh/graphql

/* esm.sh - graphql@16.11.0 */
import "/node/process.mjs";
export * from "/graphql@16.11.0/es2022/graphql.mjs";

where process is polyfilled

ref esm-dev/esm.sh#1134 (comment)

[yaacovCR]

Our 16.x.x instanceOf workflow actually assumes that we are in production mode only if process.NODE_ENV === 'production' and so in other environments, you may get dev mode if process === 'false', and you would have to set the whole thing as above, see: https://www.graphql-js.org/docs/going-to-production/

But checking out the code you actually get with esm from your link above, it looks like NODE_ENV got erased, and we are just checking globalThis.process, and if that's truthy, you get the production build, which is....interesting. Not sure I'm reading it right.

So maybe we would want to set process to true? But that wouldn't get rid of the ponyfill?

Seems like there might be a workaround. Right now, if you use "https://esm.sh/graphql?target=node" you get no ponyfill, and the same erasure of NODE_ENV, so I guess a workaround for now might be to use that link, and just make sure that process is truthy before calling that link?

In v17, we are going to switch the default to be production code, so we wouldn't need to touch the browser field there, assuming that nothing else changes.

[dimaMachina]

But checking out the code you actually get with esm from your link above, it looks like NODE_ENV got erased, and we are just checking globalThis.process, and if that's truthy, you get the production build, which is....interesting. Not sure I'm reading it right.

When loading graphql via an ESM CDN, process is accidentally polyfilled, so globalThis.process will be truthy.

So maybe we would want to set process to true? But that wouldn't get rid of the ponyfill?

I strongly suggest this change to unify behaviour when loading graphql for both usage - npm and ESM CDN.

[yaacovCR]

Thanks for bringing this up, your expertise is very helpful.

Apologies though for my obtuseness, I thought process is ponyfilled by esm.sh, not polyfilled, I tested just from my chrome console by just running

const { isObjectType } = await import('https://esm.sh/graphql');
globalThis.process;

and it doesn't seem to be setting globalThis.process => at least for me. Checking the transformed code, it imports a ponyfill and then transforms our package to use the __Process$ ponyfill instead of process. I would have guessed that was safe. It does indeed cause the ponyfilled process value to be truthy, but that is actually good (at least for us) because it forces production mode.

For example, I tested this change by pushing a branch of the compiled library to my fork with "browser": { "process": false } as suggested, see (https://github.com/yaacovCR/graphql-js/tree/test-npm)

Checking https://esm.sh/gh/yaacovCR/graphql-js@e3112d32 (the commit short hash, using the branch name seems to get the first version of the branch without the additional commit) we get:

/* esm.sh - gh/yaacovCR/graphql-js@e3112d32 */
export * from "/gh/yaacovCR/graphql-js@e3112d32/es2022/graphql-js.mjs";

And then running this in the console, we get our development version of instanceOf, which means a significance performance hit:

const { isObjectType } = await import('https://esm.sh/gh/yaacovCR/graphql-js@e3112d32');

function getFakeObjectType() {
  class GraphQLObjectType {
    get [Symbol.toStringTag]() {
      return 'GraphQLObjectType';
    }
  }
  return new GraphQLObjectType();
};

isObjectType(getFakeObjectType());

instanceOf.ts:33 Uncaught Error: Cannot use GraphQLObjectType "{}" from another module or realm.

Ensure that there is only one instance of "graphql" in the node_modules
directory. If different versions of "graphql" are the dependencies of other
relied on modules, use "resolutions" to ensure only one version is installed.

https://yarnpkg.com/en/docs/selective-version-resolutions

Duplicate "graphql" modules cannot be used at the same time since different
versions may have different capabilities and behavior. The data from one
version used in the function from another could produce confusing and
spurious results.
at le (instanceOf.ts:33:17)
at k (definition.ts:500:1)
at :12:1

[yaacovCR]

Why does it force production mode? because this code:

graphql-js/src/jsutils/instanceOf.ts

Lines 4 to 7 in bb87e73

	const isProduction =
	globalThis.process &&
	// eslint-disable-next-line no-undef
	process.env.NODE_ENV === 'production';

seems to be rewritten to:

 const isProduction = 
   globalThis.process && 
   'production' === 'production'; 

and then to

 const isProduction = 
   globalThis.process

and then to

 const isProduction = 
   __Process$

And then supplying the polyfill means that the default for node is slow unless NODE_ENV is explicitly set to production

In contrast, the default for esm.sh is production, unless you explicitly ask for dev, which is the behavior we prefer and have decided to switch to in v17.

[dimaMachina]

Thanks Yaacov for your amazing investigation!

and it doesn't seem to be setting globalThis.process => at least for me.

I might be wrong about this. Recently I’ve struggled a lot in GraphiQL 5 with importing monaco-editor via esm.sh. Keybindings that include the Cmd key don’t work on Mac devices, "Go to Definition" with the Cmd key doesn’t work either. Instead, it only works with the Ctrl key, even when using a Mac device.

That issue was caused because monaco-editor identifies device not based on the navigator.userAgent value, but primarily through process, which is ponyfilled.

I fixed it in @graphiql/react with browser field

https://github.com/graphql/graphiql/blob/7e8057dc84159777a7c34c022aa44110ef4511f7/packages/graphiql-react/package.json#L87-L91

But after I saw that while importing graphiql there is still ponyfill of process which comes from some package, and I identified that it comes from graphql

So I suggested this change in monaco-editor and here.

Feel free to close this PR if you don't want to break current behaviour, I just want to suggest avoid including unneeded ponyfills!

[yaacovCR]

But after I saw that while importing graphiql there is still ponyfill of process which comes from some package, and I identified that it comes from graphql

So I suggested this change in monaco-editor and here.

Feel free to close this PR if you don't want to break current behaviour, I just want to suggest avoid including unneeded ponyfil

Thanks so @dimaMachina much for bringing this up. I was not actually aware of how things worked with esm.sh and this was very educational.

From what I understand, the bug in graphiql would be fixed if monaco-editor updated to not check a potential ponyfill even if graphql-js kept using the ponyfill, you are suggesting that we avoid the ponyfill if we can. I think based on the above, we cannot get rid of it for v16, but we are hopeful to have it gone in v17. Closing this for now, feel free to reopen if I have this wrong or there is more you think we can do at this stage.