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

v1.0.0 incompatible with Node.js at runtime #93

Open
jaydenseric opened this issue Jun 11, 2024 · 4 comments
Open

v1.0.0 incompatible with Node.js at runtime #93

jaydenseric opened this issue Jun 11, 2024 · 4 comments

Comments

@jaydenseric
Copy link

The recent v1.0.0 release has made this package incompatible with the Node.js runtime, because of this change from a valid fully specified import path to an invalid non fully specified import path missing the file extension:

https://github.com/slicknode/graphql-query-complexity/pull/91/files#diff-720574529a576948113b0f865801527a139a756ce25203d58f98f714bfd59b76R11

With Node.js v22.3.0, here is the runtime error:

node:internal/modules/run_main:115
    triggerUncaughtException(
    ^

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '[redacted]/node_modules/graphql/execution/values' imported from [redacted]/node_modules/graphql-query-complexity/dist/esm/QueryComplexity.js
Did you mean to import "graphql/execution/values.js"?
    at finalizeResolution (node:internal/modules/esm/resolve:260:11)
    at moduleResolve (node:internal/modules/esm/resolve:920:10)
    at defaultResolve (node:internal/modules/esm/resolve:1119:11)
    at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:541:12)
    at ModuleLoader.resolve (node:internal/modules/esm/loader:510:25)
    at ModuleLoader.getModuleJob (node:internal/modules/esm/loader:240:38)
    at ModuleJob._link (node:internal/modules/esm/module_job:126:49) {
  code: 'ERR_MODULE_NOT_FOUND',
  url: 'file://[redacted]/node_modules/graphql/execution/values'
}

We do not have a bundler in our Node.js GraphQL API project, and even if we did, we would be enforcing modern ESM module resolution rules which require fully specified import paths.

We will have to keep our dependency on graphql-query-complexity at ^0.12.0 until this issue is resolved.

Because the ecosystem can't be importing both ESM and CJS versions of graphql modules, so far it has settled on only importing the CJS because that's what came first and took root. The people best positioned to effect a transition to the ESM is the graphql maintainers, who could stop publishing CJS in a new major version and only publish pure ESM and force an ecosystem migration to align to ESM instead. That was actually planned for a major graphql release, but they chickened out on it in the end.

As a package author, you have to pick either importing ESM or CJS from graphql; you can't avoid making a decision.

@jaydenseric
Copy link
Author

I see you're publishing both CJS and ESM modules in graphql-query-complexity:

https://unpkg.com/browse/graphql-query-complexity@1.0.0/dist/

Something you could consider, is having a build step when creating those artifacts that inserts .mjs file extensions in the graphql import paths when generating ESM modules, and .js when generating CJS. In the past I published a Babel plugin to help with this sort of thing:

https://github.com/jaydenseric/babel-plugin-transform-require-extensions

Nowadays in my own packages I militantly publish pure ESM and haven't used it for some time.

The catch with the above approach is that people (like me) like to consume your package as ESM, but expect it to pull in the CJS version of graphql. For us to continue using the CJS version of graphql (only to prevent duel package hazards with other ecosystem packages; our codebase is actually pure ESM) we would have to deep import the CJS version of graphql-query-complexity. Your package exports field would need to be updated to allow this, because currently it prevents any deep imports:

"exports": {
".": {
"import": "./dist/esm/index.js",
"require": "./dist/cjs/index.js"
}
},

For optimal JavaScript module design you should have package exports allowing deep imports of only the things people need anyway, and ideally remove the ability for consumers to import any other way. Force them to fall into the pit of success.

@hprathap
Copy link

hprathap commented Jul 4, 2024

+1 for this

@narthur
Copy link

narthur commented Aug 28, 2024

Has anyone found a workaround for using this plugin in an ESM project? I'm assuming this is the PR that introduced the bug: #91

@papandreou
Copy link

CC @ivome

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

4 participants