Provide a clear path on how to support graphql-js 16 and 17 at the same time as a esm and cjs library author

n1ru4l commented 2 years ago

As it seems right now graphql-js 17 will ship without a commonjs solution.

Since ESM modules can only be loaded with a dynamic import within a commonjs context that will make a lot of libraries that support multiple graphql-js versions (commonjs and ESM in GraphQL.js 16 and ESM only in GraphQL.js 17) AND both CommonJS and ESM a pain to use and maintain.

Before making the decision to only ship ESM, there should be a clear path for those libraries. Otherwise, the ecosystem around graphql-js will result in kind of a broken state.

There was no clear description provided of why graphql-js 17 will be ESM only in the PR: https://github.com/graphql/graphql-js/pull/3552

It would be great to hear the benefit and intention of dropping CommonJS support.

To me, it seems like the Node.js ESM ecosystem is not major enough to justify dropping CommonJS.

A common reason for going ESM only is the "dual package hazard" issue, where a module is loaded as both ESM and CJS in the same application resulting in subtle bugs e.g. when relying on instanceof checks.

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?

if (globalThis[`_______graphql${version}`]) {
  throw new Error("Please fix your resolutions. <explanation and link for setting yarn and npm resolution>")
}

globalThis[`_______graphql${version}`] = true

However, this would still not solve the issue of an esm-only dependency requiring esm graphql and another cjs-only dependency requiring cjs graphql...

A possible workaround could be to instruct people to do ad-hoc esm graphql to cjs graphql conversions... I already tested this method...

scripts/cjsify-graphql.js

"use strict";

const path = require("path");
const fs = require("fs");
const babel = require("@babel/core");
const cjsTransform = require("@babel/plugin-transform-modules-commonjs");
const glob = require("glob");

const graphqlPath = path.dirname(require.resolve("graphql"));
const pkgJSONPath = path.join(graphqlPath, "package.json");
const oldPkgJSONPath = path.join(graphqlPath, "package.json.backup");

const fileExists = (path) => {
  try {
    fs.statSync(path);
  } catch (_) {
    return false;
  }
  return true;
};

console.log("Need some graphql commonjs?");

if (fileExists(oldPkgJSONPath)) {
  console.log("already applied everything. all good.");
  process.exit(0);
}

const pkgJSON = JSON.parse(fs.readFileSync(pkgJSONPath, "utf-8"));
fs.copyFileSync(pkgJSONPath, oldPkgJSONPath);

for (const [exportName, exportPathName] of Object.entries(pkgJSON.exports)) {
  pkgJSON.exports[exportName] = {
    import: exportPathName,
    require: exportPathName.replace(".js", ".cjs"),
  };
}

fs.writeFileSync(pkgJSONPath, JSON.stringify(pkgJSON, null, 2));

const files = glob.sync(path.join(graphqlPath, "**", "*.js"));

for (const file of files) {
  console.log("process", file);
  const cjsRegex = /from '(.*).js';$/gm;

  fs.writeFileSync(
    file.replace(".js", ".cjs"),
    babel.transform(
      fs
        .readFileSync(file, "utf-8")
        .replace(cjsRegex, (str, match1) => `from '${match1}.cjs'`),
      {
        plugins: [cjsTransform],
        babelrc: false,
        filename: file,
      }
    ).code
  );
}

Related PR links:

yaacovCR commented 2 years ago

see #3616 which could also be a workaround for the dual-package hazard with cjs/esm compatibility

jaydenseric commented 2 years ago

Provide a clear path on how to support graphql-js 16 and 17 at the same time as a esm and cjs library author

We shouldn't. Pure ESM is a good and necessary major change; it's ok for the ecosystem to upgrade to v17 when ready and leave v16 behind.

See these comments for detailed reasons why pure ESM is good and attempting both ESM and CJS is not:

https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c#gistcomment-3850849

I'm really looking forward to the pure ESM graphql v17 release; as soon as it happens I’ll swap graphql-upload over to pure ESM also:

https://github.com/jaydenseric/graphql-upload/pull/290#pullrequestreview-981670520

If that takes too long I'll move it to pure ESM regardless.

There is no need to overthink and over-engineer all this. CJS has overstayed its welcome and we’re collectively torturing ourselves with it for no particular reason; the time is now to switch to pure ESM. There is never going to be a better time. Many of Sindre Sorhus's packages are pure ESM, as are most of mine, packages like node-fetch, the remark/rehype family of packages, and more. graphql is not the first to move to pure ESM and it sure won't be the last. Very soon people won't be able to support or use CJS even if they wanted to.

jaydenseric commented 2 years ago

If that takes too long I'll move it to pure ESM regardless.

It took too long 😂 graphql-upload is pure ESM from v16.0.0, so don't worry about downstream effects of moving graphql to pure ESM for that package as we only need/want ESM moving forwards.

It would be a massive improvement though if graphql allowed optimal JavaScript module design by publishing modules that only have single purpose default exports. For example, graphql-upload is currently using named imports from the graphql main index module:

https://github.com/jaydenseric/graphql-upload/blob/5d55705bead7e5518ddc86d2052fe2598c48a35f/GraphQLUpload.mjs#L3

import { GraphQLError, GraphQLScalarType } from "graphql";

This causes all the library to be loaded when we only need to import those two things. Ideally there would be a deep import to get only GraphQLScalarType without loading anything else; currently it's in a module that has a bunch of other stuff in it:

graphql / graphql-js

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