ESM vs CJS

[jankapunkt]

This module is, for historical reasons, still CJS-only. However, there is currently a shift in the overall ecosystem, where many packages move towards ESM-only. A direct consequence is already here: we currently cannot install amnd use chai @ 5.x as it's esm-only now. This situation will rather increase than decrease.

Therefore I was thinking what we should do:

• move to esm only, too? This will be very hard for many users as not many are ready for this step, forcing them to remain on older versions of the package
• provide multiple builds? With tools like rollup we can easily create multiple builds for different targets (ESM, CJS, AMD, UMD, iife), this guarantees compatibility but increases complexity of our toolchain
• stick with CJS as out output and use tools like babel or reify to support whatever our dependencies put on the table
• a total different way?

Let's discuss, wdyt

[Maia-Everett]

I'd rather the library didn't go ESM only. For now, I'm stuck with CJS, because NestJS currently only supports CJS (see nestjs/nest#8736).

I'd be fine with a double CJS/ESM build for now, to ease migration.

[jankapunkt]

I see this problem with many frameworks. They move much slower than libraries, that already made the switch to full esm.

[martinssonj]

Some libraries are ESM only for the latest major version and keep maintaining an older version with CJS for some time. That is also a good approach.

[opiation]

Hey there! I recently forked this library to see how much work it would be to get an ESM version up an running. You can see my branch here: https://github.com/opiation/oauth2-server/tree/chore/convert-to-esm

The tests run and all pass (with 2 changes to line numbers in an expected stacktrace). Sorry for all the whitespace changes. My local setup automatically ran prettier on everything. I can undo those changes if needed.

I haven't added any automation to bundle the code into both CJS and ESM format but I think that shouldn't be too much effort using the package.json#exports field. I would use something like:

"exports": {
  "import": "./dist/index.mjs",
  "require": "./dist/index.cjs",
  "default": "./index.js", // not sure about the appropriate default here though I don't know when it would get used
}

I could submit my convert-to-esm branch with dual CJS/ESM exports as a PR if maintainers here would be interested.

FWIW, there are other changes I plan to make including, Prettier automatic formatting, TypeScript type annotations in JS Doc comments (testable with tsc --noEmit, and extracting Node-specific dependencies into adapters to support Deno and Bun. Those would be separate but built on top of the whole ESM transition so I'd be happy to submit those as PRs as well if the ESM one is desired, reviewed, accepted and merged.

Anyways, to the maintainers here @jankapunkt et al., let me know what you think! Happy to discuss this further if any of this is close to but not exactly what you're looking for.

[dhensby]

I'm happy with option A or staying CJS - I don't think we should try to maintain two versions at once

[opiation]

Thanks @dhensby! Is this enough for consensus or are more inputs needed to accept an ESM refactor + Rollup as an approach?

[jankapunkt]

@opiation I think we are good with option A. For orientation I provide your a short summary of the most important constraints from our end:

• target ESM as main output; provide CJS support via build tool (looks like we're good with rollup)
• please do not yet target other runtimes than node; we will work on that in follow-up PRs
• consider the existing contribution guidelines (let use know if there is anything in there interfering with your contribution)
• please do not change the existing eslint config or behavior and don't introduce any new formatting or linting; we upgrade to eslint 9 + prettier in a follow-up PR, once the ESM transition is complete
• keep the current export behavior, where possible; changing internal export signatures to named exports will be done in a follow-up PR
• please add any important info about changes to the changelog but do not bump the package versions in the PR; we will do that afterwards, when publishing a release candidate
• any important info I have missed?

Thank you very much @opiation I'm very eager to review your PR soon 🚀

[opiation]

Hey there! Sorry for the radio silence. I was on vacation and prioritized family time over volunteer time.

Those criteria are fine and clear! For transparency and bus factor, here's my plan for how I'm going about this:

• Add rollup as a dev dependency
• Configure rollup to produce bundled Node code using require getting feature parity of bundled code for current Node code
• Test bundled Node code with test suite
• Rewrite require calls as import statements
• Rewrite module.exports assignments as default exports
• Test bundled code against test suite
• Add ES6 bundling
• Test new bundles in production using some ephemeral release on NPM (ideally, a short-lived ephemeral version could be released so the final result can be tested the way a consumer of the library would use it)

[jankapunkt]

Sounds very good to me! I'd love to review it.