Closed jonkoops closed 1 month ago
coveralls
commented
1 month ago | Totals | |
|---|---|
| Change from base Build 6adbc2fe4dad708280f8bc60590335a892510731: | 0.0% |
| Covered Lines: | 80 |
| Relevant Lines: | 80 |
rolandjitsu
commented
1 month ago @jonkoops how do you plan to
Additionally this is the first in a series of PRs that I would like to submit to resolve the publication issues with the package.
?
I was just trying to see how to introduce ESM support in react-dropzone/attr-accept (there is already https://github.com/react-dropzone/attr-accept/pull/66), but not sure what are the consequences of adding "type": "module" for older versions of node.
jonkoops
commented
1 month ago I was just trying to see how to introduce ESM support in react-dropzone/attr-accept (there is already react-dropzone/attr-accept#66), but not sure what are the consequences of adding
"type": "module"for older versions of node.
Older versions of Node.js will not support this. But newer versions in the 12.x series do since 12.17.0 (see 'History' in the documentation). When the type is set to module it is also required that the main field points to a valid ESM entrypoint. For example:
{
"type": "module",
"main": "./esm/index.js"
}This makes the CommonJS entry-point unreachable. So then in order to make it work again the exports field must be used to make it reachable again. So you'd have something like:
{
"type": "module",
"main": "./esm/index.js",
"exports": {
".": {
"import": "./esm/index.js",
"require": "./cjs/index.js"
}
}
}However, to properly support TypeScript we'd also have to add the following:
{
"type": "module",
"main": "./esm/index.js",
"types": "./esm/index.d.ts",
"exports": {
".": {
"import": {
"types": "./esm/index.d.ts",
"default": "./esm/index.js"
},
"require": {
"types": "./cjs/index.d.ts",
"default": "./cjs/index.js"
}
}
}
}Note that there are two sets of type definitions, one for the ESM version and one for the CommonJS version. This is a requirement for a valid package with a dual module format. You can read more about how the exports field works with TypeScript in their documentation.
Now, this is obviously very convoluted. And I would not actually recommend it. Since the exports field is widely supported by bundlers and Node.js we can omit the main field, as well as the types field in the root:
{
"type": "module",
"exports": {
".": {
"import": {
"types": "./esm/index.d.ts",
"default": "./esm/index.js"
},
"require": {
"types": "./cjs/index.d.ts",
"default": "./cjs/index.js"
}
}
}
}This works with most tooling, but might require users to make a small migration to their TSConfig:
{
"compilerOptions": {
"moduleResolution": "bundler" // Must be 'node16', 'nodenext' or 'bundler'
}
}This configuration opts the TypeScript compiler into modern package resolution using the exports field, you can read more about this in the documentation. Users should be opt-in into this resolution mechanism anyways, and the compiler will warn about this as well.
I would also recommend to drop the CommonJS entry-point to simplify things even further:
{
"type": "module",
"exports": {
".": {
"types": "./esm/index.d.ts",
"default": "./esm/index.js"
}
}
}This removes the need to compile another version and also removes the need to carry two sets of type definitions in the distribution, significantly shrinking installed package size. The only downside is that this module can no longer be imported from CommonJS, this is already fixed in Bun, and will be fixed in all LTS versions of Node.js (--experimental-require-module will be unflagged and backported).
This is the most future-proof and lightweight version of a modern NPM package. And I would like to slowly port all react-dropzone packages to this version if you agree. Let me know if you have any questions.
rolandjitsu
commented
1 month ago @jonkoops thanks for the detailed explanation. Things have changed quite a bit in the past few years since I haven't been working within this ecosystem.
After trying to make attr-accept be backwards compatible, I've realised it's too convoluted/complex to do it (a lot of the dev deps haven't migrated to ESM or haven't been updated in years - much like this package). I think it's sensible to migrate to ESM and drop support for commonjs.
What'll most likely try to do, is that the last patch or version we release here or in the other libs, we''l mark as deprecated and ask to migrate to Node 18/20+ (ESM) and at the same time we release a major with ESM support and dropped commonjs support.
github-actions[bot]
commented
1 month ago :tada: This PR is included in version 1.0.0 :tada:
The release is available on:
Your semantic-release bot :package::rocket:
jonkoops
commented
1 month ago Sounds like a plan, I'll see if I can refactor file-selector to a a point we're both happy with and then we can use that as a basis for the other libs :+1:
jonkoops
commented
1 month ago Hmmm, not sure why, but the semantic release bot versioned this 1.0.0 because of a breaking change. Even though semantic versioning clearly states that pre-1.0 breaking changes are allowed without a major bump...
What kind of change does this PR introduce?
Did you add tests for your changes?
If relevant, did you update the documentation?
Summary Removes the ES5 distribution from the package to slim down the installed package size. Additionally this is the first in a series of PRs that I would like to submit to resolve the publication issues with the package.
Does this PR introduce a breaking change? Yes, but this should not affect any of the supported versions of Node.js (
>=12) and all modern browsers have implemented ES2015 for several years now, so this should not affect any real production environments.Other information None.