feat: replace instanceOf with unique Symbol checks

[n1ru4l]

This is the first step for a potential reintroduction of CommonJS alongside ESM that avoids the dual package hazard instanceOf issues.

In addition, I think it would make sense to introduce and assert a flag on globalThis in order to avoid multiple GraphQL.js versions being used together (unless there is a valid use case) that should be importet at the top of each entry-point file.

entry-shim.ts

const versionSymbol = Symbol.for("loaded-graphql-version")

if (globalThis[versionSymbol] && globalThis[versionSymbol] !== version) {
  throw new Error("Tried loading two different GraphQL versions. Please resolve your resolutions.")
}

globalThis[Symbol.for("graphql-version")] = version

Update: I don't think we should manually assert the GraphQL version with an entry-shim as described above. Resolving a single version of GraphQL is the responsibility of the package manager.

The motivation behind this PR is the reality that CommonJS and ESM will co-exist for some more time until the whole ecosystem catches up. See #3603

---

Related:

• Provide a clear path on how to support graphql-js 16 and 17 at the same time as a esm and cjs library author #3603
• Separate interfaces from implementations for exported classes #3616

[netlify]

✅ Deploy Preview for compassionate-pike-271cb3 ready!

Name	Link
🔨 Latest commit	5bdfe61
🔍 Latest deploy log	https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/62b0c97843924f00087bca26
😎 Deploy Preview	https://deploy-preview-3617--compassionate-pike-271cb3.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[yaacovCR]

Makes sense to me in terms of separating version checks from instanceOf checks, the two are related, but ultimately separate concerns. I guess the version check changes should be part of this PR if old instanceOf behavior is being deprecated.

[Fundamentally, I don't think we should be doing version checks. If the TS type signatures align, imo that is the package manager's responsibility and graphql-js should get out of the way.

But considering we currently do, I guess it is reasonable to continue.]

[yaacovCR]

Review requested? Approach seems great -- although we also need to decide about whether/where/how to enforce common version.

Generally, I think we need feedback from @IvanGoncharov or perhaps the larger graphql-js-wg or even the main graphql-wg to figure out which way forward on esm only.

[IvanGoncharov]

I suggest making it a static property otherwise it consumes memory on every instance.
Also, I suggest having a single static property with symbol graphql@{git_hash} as key and enum similar to what @yaacovCR has in #3616

[n1ru4l]

I am not sure whether making it a static property would make sense as another issue this might solve in the future is to have GraphQLError like-objects that are not instances of the GraphQLErro class. I am sure @yaacovCR also has opinions on that.

The following seems way too specific for classes:

maybeError.constructor?.[isGraphQLErrorSymbol] === true

vs

maybeError[isGraphQLErrorSymbol] === true

I am not sure whether a boolean per instance is that big of an issue and a memory monster. 🤔

---

Regarding: "I suggest having a single static property with symbol graphql@{git_hash} as key and enum similar to what @yaacovCR has in #3616"

I am not sure whether I get this right, but do you assume that each symbol should be scoped to a git hash commit in order to avoid cross-version usage?

In that case, I would rather propose the solution mentioned in #3603 (comment) and raise an error as soon as a second GraphQL.js version is loaded which IMHO should be a separate pull request.

Instead could be another solution to add something like this to the "entry" modules to ensure only one version of GraphQL.js is loaded? Are there use-cases in which you would want to run two graphql-js versions alongside each other?

import { version } from './version';

interface GlobalThisCast {
 loadedGraphQLVersion?: undefined | string;
}

const alreadyLoadedGraphQLVersion = (globalThis as GlobalThisCast)
 .loadedGraphQLVersion;

if (
 alreadyLoadedGraphQLVersion !== undefined &&
 alreadyLoadedGraphQLVersion !== version
) {
 throw new TypeError(
   `Tried loading graphql-js '${version}' alongside '${alreadyLoadedGraphQLVersion}'.` +
     '\nPlease revisit your resolutions and installed graphql-js versions and make sure only a single version is loaded at a time.',
 );
}

(globalThis as GlobalThisCast).loadedGraphQLVersion = version;

[n1ru4l]

I am not sure whether we need this check at all. Our runtime code should not address wrongly configured/installed dependencies. Only installing and loading a single version of graphql-js should be a package manager's concern, not ours.

[yaacovCR]

If we leave the check in, we may need to keep it working the way it does, ie by verifying the version used when passing objects around.

This is because some libraries could in theory want to support multiple versions of graphql as long as they ensure that the right versions talk to each other.

I am envisioning a version of graphiql that lets you dynamically switch between versions of graphql js or something like that.

[yaacovCR]

But I generally agree with @n1ru4l that we should seriously consider removing this check. It made sense for the time when the library moved from being a regular dependency to a peer dependency, but that time was long ago.

[IvanGoncharov]

We have two use cases that this change potentially can solve:

1. CJS/ESM
2. passing Schema to workers, see Getting multiple instances of graphql error despite only having a single version #2801

However #2 is need to be confirmed, see: #2801 (comment)

Do you know any other use case this change solves beyond ESM/CJS?

[IvanGoncharov]

I'm asking because if we just addressing CJS/MJS here, we need to compare it with the solution from #3361 or other similar solutions.

[n1ru4l]

@IvanGoncharov I am not a big fan of #3361 as it would require using resolution overwrites. On yarn using resolutions is straightforward and handy. On npm, you will have to use third-party packages. 🙁 (https://www.npmjs.com/package/npm-force-resolutions)

[saihaj]

I do like the idea of Symbol also being used for version checks

[dotansimha]

I like the Symbol solution. Seems like a pretty straightforward solution.
Since Symbol.for will work regardless of where it was executed from, I think it will also solve the multiple instances issue.