-
Notifications
You must be signed in to change notification settings - Fork 337
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
Conversation
There was a problem hiding this 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.
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 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 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.
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.
Yeah, perhaps the messaging here is too aggressive. How would you feel about:
|
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:
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. |
Very well, I think that is fair. Perhaps a simple message in the README directing folks towards the ESM version would still be acceptable?
This is a peculiar choice, especially for So I'd argue that indeed ESM is not a good fit for that particular library. That said, I think
This is a good point actually. Shall I open up some PRs to add ESM support to those as well? |
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.