attr-accept
attr-accept copied to clipboard
react-dropzone
build!: add support for ESM and drop support for CommonJS
What kind of change does this PR introduce?
- [ ] Bug Fix
- [ ] Feature
- [ ] Refactoring
- [ ] Style
- [x] Build
- [ ] Chore
- [ ] Documentation
- [ ] CI
Did you add tests for your changes?
- [x] Yes, my code is well tested
- [ ] Not relevant
If relevant, did you update the documentation?
- [ ] Yes, I've updated the documentation
- [x] Not relevant
Summary
Fully migrated the package to ESM and dropped support for CommonJS.
Does this PR introduce a breaking change?
Yes. Any use of this package that does not support ESM will no longer work.
Other information
What is the reasoning for ESM only? I'm trying to catch up but I'm on vacation at the moment. Can I get some context @rolandjitsu ?
Checkout https://github.com/react-dropzone/attr-accept/pull/66 and the original https://github.com/react-dropzone/attr-accept/pull/64 for some context.
I've initially tried to keep support for commonjs, but a bump of node was needed, then I got into some issues with the lock file, which needed an update as well. Then other deps caused issues, etc.
To me it kind of seems that moving to ESM is the right thing to do, despite the fact that the consequence of that means some users (not sure if there is a way to show a percentage) will need to move to ESM as well, which I think that's inevitable (given that even node 18 will reach EOL next year some time - https://nodejs.org/en/about/previous-releases).
This simplifies the package in terms of build (there's nothing to build anymore) and size (just a couple hundred bytes vs kb).
We can keep the v2 version as a maintenance branch (we can update the release workflow to release from a v2.2.x branch) and patch it when necessary (we've hardly had any updates within years), but it'll require some work to make it work with pinned down versions of packages, and v3 will be fully ESM.
Let me know if that makes sense.
What is the reasoning for ESM only? I'm trying to catch up but I'm on vacation at the moment. Can I get some context @rolandjitsu ?
Checkout #66 and the original #64 for some context.
I've initially tried to keep support for commonjs, but a bump of node was needed, then I got into some issues with the lock file, which needed an update as well. Then other deps caused issues, etc.
To me it kind of seems that moving to ESM is the right thing to do, despite the fact that the consequence of that means some users (not sure if there is a way to show a percentage) will need to move to ESM as well, which I think that's inevitable (given that even node 18 will reach EOL next year some time - https://nodejs.org/en/about/previous-releases).
This simplifies the package in terms of build (there's nothing to build anymore) and size (just a couple hundred bytes vs kb).
We can keep the v2 version as a maintenance branch (we can update the release workflow to release from a v2.2.x branch) and patch it when necessary (we've hardly had any updates within years), but it'll require some work to make it work with pinned down versions of packages, and v3 will be fully ESM.
Let me know if that makes sense.
I agree that shipping ESM is the right direction, but don't think we should just drop CommonJS entirely. Could we not just leverage swc to build out both and with proper exports/tree shaking it would still provide a smaller package? I can take a shot at it in another PR
@rxmarbles I'm going through the process of keeping commonjs support with the react-dropzone repo and it doesn't seem like a straightforward thing to do (mostly because some deps haven't been updated in ages).
This repo may be different though as it's a lot simpler.
Still, given that most, if not all, modern bundlers and browsers support ESM, I'd really prefer to keep it simple and fully migrate to ESM with a major version bump. Patches/Fixes can be delivered via maintenance branches when needed.
Still, given that most, if not all, modern bundlers and browsers support ESM,
I hate to sound like a broken record, but this is not the issue. The issue is that anyone still writing CommonJS code won't be able to use this package anymore (at least, via require). Somewhat related, I heard recently that create-react-app's default behavior is still CommonJS.
Update: Hmm, but maybe when they're using a bundler this doesn't matter, because the ESM will get translated to CommonJS. So maybe it's just for people not using a bundler, which is probably a small group.
I'd really prefer to keep it simple and fully migrate to ESM with a major version bump.
This still might be a reasonable decision though! This search and this (25 matches) indicate that there aren't that many users still using CommonJS, other than react-dropzone. For contrast, here are the searches for ESM: single quotes, double quotes (about 200 matches).
I'm going through the process of keeping commonjs support with the react-dropzone repo
But if you're planning to keep CJS support in react-dropzone, don't you have to do so also for all dependencies, including attr-accept? Or are you ending up deciding not to do it for react-dropzone?
I could also give a try using esbuild to make a simple dual ESM/CJS build PR. It's usually pretty easy in my experience. Or you could just use #64, which is not pretty but seems to work fine?
@rolandjitsu @edemaine I've opened #77 as an alternative for you all to review. Happy to continue the discussion here if necessary but wanted to show that it is still possible to support both. Eventually I would love for 1 spec to be supported in both Node and Browsers (one day node will catch up, lots of folks are trying 😄)
@rolandjitsu @edemaine I've opened #77 as an alternative for you all to review. Happy to continue the discussion here if necessary but wanted to show that it is still possible to support both. Eventually I would love for 1 spec to be supported in both Node and Browsers (one day node will catch up, lots of folks are trying 😄)
Thanks @rxmarbles . I have left some thoguhts/comments. Btw, as far as I know, the ESM spec is being adopted by most/all browsers and bundlers.
@edemaine thanks for taking the time to look into the numbers.
I hate to sound like a broken record, but this is not the issue. The issue is that anyone still writing CommonJS code won't be able to use this package anymore (at least, via require). Somewhat related, I heard recently that create-react-app's default behavior is still CommonJS.
Update: Hmm, but maybe when they're using a bundler this doesn't matter, because the ESM will get translated to CommonJS. So maybe it's just for people not using a bundler, which is probably a small group.
I would assume that, the same way ppl use transpilers today to convert from typescript to js, they would also use them to consume ESM if their setup is not capable yet.
This still might be a reasonable decision though! This search and this (25 matches) indicate that there aren't that many users still using CommonJS, other than react-dropzone. For contrast, here are the searches for ESM: single quotes, double quotes (about 200 matches).
That probably only shows use in public repos? I assume that there are other groups that haven't published anything on github.
But if you're planning to keep CJS support in react-dropzone, don't you have to do so also for all dependencies, including attr-accept? Or are you ending up deciding not to do it for react-dropzone?
I tried to make the transition easy, but it looks like with the current way I tried to go about it, there's some issues that we'd need to figure out. The biggest being the documentation generation, but I did plan to migrate that to something else (docusaurus or storybook or something that's being actively worked on).
I'm still thinking to make the unpopular decision of dropping commonjs as a breaking change.
I could also give a try using esbuild to make a simple dual ESM/CJS build PR. It's usually pretty easy in my experience. Or you could just use https://github.com/react-dropzone/attr-accept/pull/64, which is not pretty but seems to work fine?
@rxmarbles also has a PR now and #64 is also ok. It all comes down to deciding to support commonjs or not.
The plan for this is to merge into beta and continue there. We'll keep the current version of this lib as is for now.
@rolandjitsu got a proposal for running the tests on all versions of Node.js see #82
@rolandjitsu got a proposal for running the tests on all versions of Node.js see #82
Thanks! Merged 🎉
:tada: This PR is included in version 3.0.0 :tada:
The release is available on:
Your semantic-release bot :package::rocket:
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
🎉 This PR is included in version 3.0.0 🎉
The release is available on:
Your semantic-release bot 📦🚀
What? I expected a beta release here 😕
Not sure why, but semantic release decided to tag this with 3.0.0 which should have been 3.0.0-beta.1 and not the final tag, @rolandjitsu perhaps you can still pull this release?
Not sure why, but semantic release decided to tag this with
3.0.0which should have been3.0.0-beta.1and not the final tag, @rolandjitsu perhaps you can still pull this release?
Yeah. I'm also confused. It's probably the current config which we'll need to change to the default as specified in https://semantic-release.gitbook.io/semantic-release/usage/configuration#branches.
I could delete the release ...
Yeah, let's pull it and fix the configuration. I'll create a PR to fix it up if you can pull the NPM package.
Hmmm ... it looks like the release went into the npm beta channel and marked as pre-release on github:
But it omitted the beta version from the version string. Definitely a config thing.
Crap ...
✗ npm unpublish "[email protected]"
npm error code E405
npm error 405 Method Not Allowed - PUT https://registry.npmjs.org/attr-accept/-rev/23-202eefe40bdacc7cbdc7b19b8f03d461 - You can no longer unpublish this package.
npm error Failed criteria:
npm error has dependent packages in the registry
npm error
npm error Please deprecate it instead:
npm error npm deprecate -f '[email protected]' "this package has been deprecated"
npm error To learn more about our unpublish policies, see https://www.npmjs.com/policies/unpublish
npm error A complete log of this run can be found in: /Users/rolandgroza/.npm/_logs/2024-11-11T12_18_42_605Z-debug-0.log
I did deprecate it ... in hopes it'll let me unpublish it ... but no.
Ah bummer, at least it was not tagged as the current release so it will not be installed automatically. We'll just have to use 3.0.1 as the first stable then.
Ah bummer, at least it was not tagged as the current release so it will not be installed automatically. We'll just have to use
3.0.1as the first stable then.
According to https://docs.npmjs.com/policies/unpublish:
Regardless of how long ago a package was published, you can unpublish a package that meets all of the following conditions:
- no other packages in the npm Public Registry depend on it
- it had less than 300 downloads over the last week
- it has a single owner/maintainer
I suppose pt 3 is the issue here. Unless we already got 300+ downloads and the version is already used in other packages (how?).
Ah bummer, at least it was not tagged as the current release so it will not be installed automatically. We'll just have to use
3.0.1as the first stable then.According to https://docs.npmjs.com/policies/unpublish:
Regardless of how long ago a package was published, you can unpublish a package that meets all of the following conditions:
- no other packages in the npm Public Registry depend on it
- it had less than 300 downloads over the last week
- it has a single owner/maintainer
I suppose pt 3 is the issue here. Unless we already got 300+ downloads and the version is already used in other packages (how?).
Nope. I tried removing the 2 other maintainers in hopes it'll work ... sorry @rxmarbles and @okonet (I can add you back).
:tada: This PR is included in version 3.0.0-beta.1 :tada:
The release is available on:
Your semantic-release bot :package::rocket:
I've reached out to support to see if they'd be willing to remove that version. Otherwise we won't be able to use it and the 3.0.0 release will likely fail when it tries to publish on npm.
NPM support mentioned that it cannot be unpublished. And even if we can, we can never reuse the version. So the v3 release will fail with npm, but we should be able to make it to 3.0.1 by adding some noop fix commit.
Unfortunate, but that is what we'll be going for then. Such a shame that semantic-release is so aggressive. Perhaps we should find a way to configure it so we can ack the release before it goes through, or trigger it only with a manual workflow.