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)