Output both CJS & ESM

[acidoxee]

Description

In response to #378, I've started modernizing this package and writing ESM by default. I'm using tsc to output both CJS & ESM entrypoints, along with type definitions.

I don't believe using a full-fledged bundler is necessary since the package is quite small. Also, since the package was authored in JS, I've kept it that way instead of migrating to a full TS codebase. I've removed the index.d.ts manual type definition file though, leveraging just a src/types.d.ts file to host source types (it's handier than editing and sharing types in JS files). This file is copied manually during the build:* npm scripts since tsc doesn't do it, but "real" type definitions are now entirely handled by tsc, ensuring accuracy regarding sources and module resolution.

Build artifacts are now contained in a classic dist folder, where there are cjs and esm entrypoints. The CJS one has an additional package.json file with { "type": "commonjs" } inside, since we're now in an ESM package by default at the root and tsc didn't output .cjs/.d.cts extensions. Those entrypoints are properly exposed through subpath exports with the exports field in the root package.json.

The previous TS config wasn't checking source files, only the manual declaration file and the single test written in TS, so I believe a LOT of existing errors were ignored. So I've introduced a few different configs, each aimed at a different outcome:

• tsconfig.tests.json for tests, which is more or less what the previous config did. It's still more extensive since it also checks JS files, whereas the previous config only checked the single tests/ts-definitions.tests.ts file
• tsconfig.build-*.json for each build output
• tsconfig.json for source, which contains almost all files and serves CI and IDE checks

The TS config for sources doesn't emit anything, it's just here for typechecking. All configs extend a single tsconfig.base.json for brevity, which itself extends @tsconfig/node14. I've used this version since this seemed to be the lowest Node version you're aiming to support.

At the time of writing this, there's still some work to do:

• I've silenced existing TS errors with // @ts-ignore comments. I've typed what was easy to infer for me, but for the rest I believe existing maintainers will have a way easier time than me, who's discovering the codebase. I think a lot of errors will disappear once typings of "original" sources will be completed, since it ripples through the codebase
• the examples folder hasn't been touched for now, nor is it included in typechecking right now. Since the source codebase is now ESM, examples in CJS that imported relative source files (like const xxx = require('../../src');) won't work anymore. I suggest either rewriting examples in ESM to leverage "real" source imports, or faking the package imports with require('jwks-rsa') like we were not in the package's codebase. Also, I'm pretty sure some existing examples don't work, but I've not taken the time to dive in there yet

Do note that I've removed the "default" export from src/index.js, because those really don't play well between bundlers/runtimes. I've followed the recommendations from AreTheTypesWrong, and I've updated the README to show a named import and new instantiation of the client class instead. This is a breaking change and should warrant a new major version bump.

References

Related to #378.

Testing

I've added CircleCI and GitHub Action jobs for typechecking and verifying that built outputs have proper types and resolution, thanks to AreTheTypesWrong's CLI. At this point I've just copied what was already there and added the new scripts, I've no idea where/whether we'll want those checks here.

Those checks can be performed locally as well with the new types:check and build:check npm scripts.

• This change adds test coverage for new/changed/fixed functionality (I've checked this box since the aforementioned scripts should do quite well, but we might want to add some more tests around build outputs, like building them and them importing them for real in ESM and CJS tests to verify they work)

Checklist

• I have added documentation for new/changed functionality in this PR or in auth0.com/docs (not done yet, it should be advertised that this package will be dual CJS/ESM, and that there's a breaking change)
• All active GitHub checks for tests, formatting, and security are passing (?)
• The correct base branch is being used, if not the default branch (?)

[adamjmcgrath]

Thanks for your effort here @acidoxee

This is a breaking change and should warrant a new major version bump.

We're not looking to make a breaking change on this library currently and are happy with the current status of it, so apologies - we wont be proceeding with this PR.

Happy to consider a much simpler PR that adds ESM support and does not require a major

[acidoxee]

My apologies for assuming you'd be OK with a breaking change, I did not ask prior to writing the PR as I should have.

Although to reassure you, the breaking change I'm suggesting is actually a tiny fraction of the PR (quite literally 3 lines of code removed), since the only reason for it was to remove the "default" export of the lib, which is that factory:

node-jwks-rsa/src/index.js

Lines 8 to 10 in 6b89c6b

	module.exports = (options) => {
	return new JwksClient(options);
	};

Since the JwksClient class is already exported and available to all, the only breaking change introduced in this PR is that people would have had to do this change in their codebase:

- const jwksClient = require('jwks-rsa');
+ const { JwksClient } = require('jwks-rsa');

- const client = jwksClient({
+ const client = new JwksClient({
  jwksUri: 'https://sandrino.auth0.com/.well-known/jwks.json',
  requestHeaders: {}, // Optional
  timeout: 30000 // Defaults to 30s
});

That seemed quite acceptable to me since this small change made the setup of dual CJS/ESM simpler, but I'm sorry I've assumed that. To be honest I'm not quite sure why that factory exists in the first place, but that's none of my business if you want to keep it that way. Would you be open to keeping the PR and going further if I managed to remove the breaking change?

[adamjmcgrath]

Hi @acidoxee - sure ok, could you remove the breaking change

We have a history of accepting PRs that attempt to add ESM support and end up breaking in various bundlers that we haven't checked, so you might have to be patient while I find the time to test this out on some

[d3c0d3dpt]

Hey @acidoxee , is there any contribute I can make to get the ball rolling on this again?

[acidoxee]

Hi @d3c0d3dpt 👋 Thanks for taking an interest, I haven't had time to return to this PR until last time, but if I remember the remaining things were:

• Rebase everything since there are now conflicts
• Remove the breaking change I've introduced (which is explained in the PR description and in my previous comment). If rolled back, this will probably make AreTheTypesWrong's CLI tests error, for the reasons I've mentioned regarding default export issues between CJS and ESM. One solution I had in mind would be to try out https://github.com/isaacs/tshy to create ESM/CJS outputs, I reckon they try to handle those edge-cases
• Perform the remaining tasks, such as fixing TS errors (which I've silenced for now). I don't think all of them are mandatory though (TS for instance, since I've just made it so that files previously ignored are now included. So IIRC every new error was "already there", it's just that the TS config was not picking up a lot of files)

[acidoxee]

Hi everyone, it's been a while, and unfortunately our bundles are still quite heavily bloated with this lib and the CJS version of its dependencies (jose being by far the heaviest IIRC). Since then Node v22 has shipped with experimental support to require ES modules from CJS:

The CommonJS module require currently only supports loading synchronous ES modules when --experimental-require-module is enabled.

See also this blog post from the author of the original PR.

Support is experimental and only present in v22 (I reckon it's unlikely this feature will be backported to other versions given its complexity), so shipping ESM-only isn't viable at this very moment if you wish to support LTS releases and CJS. But v22 will become the active LTS in October, which is quite soon.

@adamjmcgrath do you think preparing to ship native ESM (and ESM only) around this period would be acceptable? It would be a huge win for us to benefit from native ESM for this package and its dependencies, and it seems the rest of the ecosystem and Node itself are finally making decent progress regarding ESM migration. I'm still willing to assist you towards this path if you're interested, both for the library's code itself and for the tools around it.

Edit: I was wrong, Node did just add experimental support to require an ES module graph in v20.17.0, so a ton of libs will soon be able to ditch CJS completely as long as they don't use top-level await and only aim to support LTS+ versions of Node.

[acidoxee]

Given the lack of interest, I'll close my PR and the related issue, and delete my fork.

I've personally "cut the middle-man" and went with using jose directly, which is just a bit lower-level, but not that hard to use directly after all, and I have to say it's incredibly well-maintained and up-to-date regarding ESM and other ecosystem practices overall. By doing so, we've been able to shave between ~40kB and ~150kB on our lambdas, which is huge for cold starts.

Still, thanks for having taken a peek at my issue and PR 👋