@urql/exchange-execute issues when using with 📦s that import `graphql` from root

[oreqizer]

Hi! 👋

Firstly, thanks for your work on this project! 🙂

Today I used patch-package to patch @urql/[email protected] for the project I'm working on.

I have a project that uses @graphql-tools/schema to build schema. The project imports graphql from the root, as:

import { GraphQLEnumType /* ... */ } from 'graphql';

Version v16.x of graphql, however, has no exports field in their pancake.json, only module and main:

{
  // ...
  "main": "index",
  "module": "index.mjs",
  // ...
}

This does not work for packages without "type": "module", such as mine. Now this creates problems, because non-module apps doing root imports from graphql then import CJS modules implicitly. However, the @urql/exchange-execute package does explicit .mjs imports.

This causes two distinct versions of the GraphQL executor to be imported, causing an error:

 'Error: Cannot use GraphQLSchema 
 ...
 from another module or realm.\n' +
        '\n' +
        'Ensure that there is only one instance of "graphql" in the node_modules\n' +
        'directory. If different versions of "graphql" are the dependencies of other\n' +
        'relied on modules, use "resolutions" to ensure only one version is installed.\n' +
        '\n' +
        'https://yarnpkg.com/en/docs/selective-version-resolutions\n' +
        '\n' +
        'Duplicate "graphql" modules cannot be used at the same time since different\n' +
        'versions may have different capabilities and behavior. The data from one\n' +
        'version used in the function from another could produce confusing and\n' +
        'spurious results.'
    }

This is an issue on graphql package's side by not having an "exports" field in their pancake.json.

While I know this is not an issue in your package, I just want to post it here so y'all are aware of it and maybe note it as a temporary workaround.

Here is the diff that solved my problem:

diff --git a/node_modules/@urql/exchange-execute/dist/urql-exchange-execute.mjs b/node_modules/@urql/exchange-execute/dist/urql-exchange-execute.mjs
index 240b63a..2705e00 100644
--- a/node_modules/@urql/exchange-execute/dist/urql-exchange-execute.mjs
+++ b/node_modules/@urql/exchange-execute/dist/urql-exchange-execute.mjs
@@ -1,8 +1,8 @@
 import { share as e, mergeMap as r, filter as n, takeUntil as t, make as o, merge as i } from "wonka";
 
-import { subscribe as u } from "graphql/subscription/index.mjs";
+import { subscribe as u } from "graphql/subscription/index.js";
 
-import { execute as a } from "graphql/execution/execute.mjs";
+import { execute as a } from "graphql/execution/execute.js";
 
 import { getOperationName as c, makeResult as f, makeErrorResult as l, mergeResultPatch as v } from "@urql/core";
 

This issue body was partially generated by patch-package.

[kitten]

I really wish they'd just get the migration over with 😞 I believe we mostly switched to graphql/*.mjs direct imports for the most part — I'm not sure if we're also doing graphql/index.mjs. That said however, we aren't taking into account that some packages will still cause direct from 'graphql' imports which then resolve to CommonJS instead.

I suppose we have two options here; either, we provide three files per entrypoint, which likely won't vibe with Vite, we switch to graphql/index.mjs instead, which likely will cause issues with third-party libraries that'll still resolve CommonJS, or we switch all of our mjs ESM files over to graphql direct imports and hope that Webpack has gotten better at treeshaking 🤔

Alternatively, we could always link against the non-mjs files, i.e. always import CommonJS. This actually seems like a rather attractive option to me, since Node.js will never automatically use the mjs files just yet, so we can likely do that. We should probably also then swith from from 'graphql' to from 'graphql/index.js where applicable.

We've recently changed this for @urql/exchange-execute, and we could do this in a two-step fashion. Do it for that package first, since we already changed it recently, then if it works, apply it to all others.

[oreqizer]

IMO importing directly from graphql and relying on bundling tools to do their job would achieve the best compatibility with other libraries.

Not optimal, but probably the most "straightforward" and consumer-friendly option

[kitten]

It's unfortunately not always as simple as that. It is tricky — in isolation I would agree with this. However, it's not that simple because we're adhering to ESM + Node ESM as much as possible. That means that we have different "degrees" of adherance per package, e.g.React packages must not use "exports" and go from exports-ESM to ESM-ish. Those packages for instance then have a different "transition point" from exports-ESM to ESM. Other packages have non-Node ESM immediately, or Node ESM immediately. Some packages have sub-packages and folders.

On top of that, older versions of Webpack have some resolution bugs and quirks. Older versions of Node have Node ESM resolution bugs and quirks. Webpack and other bundlers have issues treeshaking versions of graphql. Different versions of graphql have a different file structure and treeshake differently (meaning, not reliably)

Hence, we are doing a lot out of the box to get the best possible outcome for users immediately. That means sometimes our packages "overstep" this line and break in a very specific case. Here for instance, the very specific case is Node 16 in ESM mode from a type: "module" package/file imports a Node ESM @urql/* package that then goes to graphql without @urql/core. So, that's the level of specificity we're unfortunately dealing with 😅🥲

Hence, when we adjust and fix these things, we also have to consider how it affects our other packages, all other scenarios, and so on... We don't swing to the other end and, say, ditch the granular imports for graphql.js, because graphql.js is ultimately not there yet — not us. The issue with these cases is that we have so much width and we know that most users (and it could even just be a fraction of our users) won't actually improve their bundling/imports if we weren't this granular and treeshaking does mess up, for instance.

Edit: I hope this makes sense btw 😅 ❤️ I'm just writing it down now, since I don't think we ever explicitly have before

[oreqizer]

thanks for the lengthy explanation! I guess I'll just resort to patch-package until graphql fixes their stuff