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

Deprecate CommonJS distribution in favor of ESM #251

Closed
wants to merge 1 commit into from

Conversation

jonkoops
Copy link
Contributor

@jonkoops jonkoops commented Nov 5, 2023

This PR deprecates the CommonJS distribution and recommends to use the ESM version instead. This does not remove the CommonJS functionality, but simply warns users that it might go away in the future, and recommends them to start upgrading.

The reason for this is that the JavaScript ecosystem is slowly moving away from CommonJS to more standard ESM, and many packages have already started to ship as 'ESM only'. This helps prepare users for that coming change, without immediately forcing them to move right away.

@jonkoops jonkoops requested a review from a team as a code owner November 5, 2023 13:09
Copy link
Member

@frederikprijck frederikprijck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We wont drop cjs in a non major anyway, and we currently have no plan to drop cjs nor do we have any plans for a future major so im not sure i understand why we should deprecate it.

Additionally, we should not inject warnings for all users, that looks a bit to aggresive for something we don't even have any plans to get rid of at the moment.

I think we can recommend people to use ESM, but i don't feel like we should officially deprecate anything here just for the sake of deprecating.

@jonkoops
Copy link
Contributor Author

jonkoops commented Nov 5, 2023

We wont drop cjs in a non major anyway, and we currently have no plan to drop cjs nor do we have any plans for a future major so im not sure i understand why we should deprecate it.

My motivation for this is to set users on a path to success. A lot of new (and existing) users will not really understand the differences between ESM and CommonJS, and perhaps choose to use CommonJS simply because they don't know of the alternatives.

With the JavaScript ecosystem moving more to ESM every day, I think it's the responsibility of large packages, such as jwt-decode to actively advertise to users what the way forward will be. This is especially true that if you consider that having any ESM dependency in a Node.js environment will force users to adopt ESM fully. This is because ES modules cannot be loaded from CommonJS modules.

I agree the messaging here is strong, but the consequences for users investing in CommonJS are severe later on when they suddenly have to migrate all of their existing code to a different module system, simply because of a dependency upgrade. This is not an exclusive problem to jwt-decode, but I think that there is a responsibility here to educate users, considering the massive user-base (~4 million downloads a week).

The second part of my motivation is maintenance, if we eventually were to drop the CommonJS distribution we'd be left only with a single distribution. This means that instead of compiling the code we can ship it straight from the source files, and generate type definitions from JSDoc annotations, eliminating the need for TypeScript files and source maps (much like Svelte), reducing the package size in the process.

Additionally, we should not inject warnings for all users, that looks a bit to aggresive for something we don't even have any plans to get rid of at the moment.

In this case, the warning is only injected into the CommonJS bundle. In practice this means that only users of Node.js that have not opted into the ESM version will see this message. Users that load the code directly in a browser or through a bundler will not be affected, as the ESM version will be loaded by default here.

I think we can recommend people to use ESM, but i don't feel like we should officially deprecate anything here just for the sake of deprecating.

Yeah, perhaps the messaging here is too aggressive. How would you feel about:

Using CommonJS distribution of 'jwt-decode' is not recommended, to avoid breaking changes in the future we recommend using the ESM distribution instead.

@frederikprijck
Copy link
Member

We just went through a major version update, making some (arguably smaller) breaking changes. We also invested in putting TypeScript in place as well as try and ensure we have a correct ESM+CJS distribution (thanks again for helping us ship that).

This seems to work fine now, and we would like to keep it as such for the foreseeable future, as there is no issue with keeping it.

Keeping CJS bundle also aligns with our other NodeJS libraries:

but I think that there is a responsibility here to educate users, considering the massive user-base (~4 million downloads a week)

Even though I do not necessarily disagree, alot of those 4M users could just as well be customers who do not appreciate going through unnecessary breaking changes. So we always try to balance DX and maintenance while ensuring we do not unnecessarily cause friction for our customers. I think with the latest major version, we did a very good job in improving the quality of the library, and I do not see any direct need to deprecate CJS.

Additionally, some libraries that went with ESM only, noticed some backlash and even ended up with a CJS fork.

So long story short, let's keep it the way it is currently, and not follow up with an immediate deprecation of something we still actively ship in all of our other node libraries.

Closing this, as I think we do not want to deprecate this, nor should we add a runtime warning to our bundle for situations that work perfectly fine.

@jonkoops
Copy link
Contributor Author

jonkoops commented Nov 6, 2023

Very well, I think that is fair. Perhaps a simple message in the README directing folks towards the ESM version would still be acceptable?

Additionally, some libraries that went with ESM only, noticed some node-fetch/node-fetch#1263 and even ended up with a CJS fork.

This is a peculiar choice, especially for node-fetch, considering that is literally a polyfill for older versions of Node.js that do not support the Fetch API. Also this was done in 2021? Those were different (horrible) times in terms of ESM support in Node.js. Seems like that fork has very little actual users though.

So I'd argue that indeed ESM is not a good fit for that particular library. That said, I think jwt-decode, being mostly a library that targets modern browsers and Node.js versions, is a different case.

So long story short, let's keep it the way it is currently, and not follow up with an immediate deprecation of something we still actively ship in all of our other node libraries.

This is a good point actually. Shall I open up some PRs to add ESM support to those as well?

@jonkoops jonkoops deleted the deprecate-commonjs branch November 6, 2023 16:07
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

Successfully merging this pull request may close these issues.

None yet

2 participants