search

react-dropzone / attr-accept

JavaScript implementation of the "accept" attribute for HTML5 input element
MIT License
110 stars 24 forks source link

build!: add support for ESM and drop support for CommonJS #75

Open rolandjitsu opened 1 week ago

rolandjitsu commented 1 week ago

What kind of change does this PR introduce?

Did you add tests for your changes?

If relevant, did you update the documentation?

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

edemaine commented 1 week ago

Looks good to me! (having read but not tested the changes)

rolandjitsu commented 6 days ago

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.

rxmarbles commented 2 days ago

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

rolandjitsu commented 8 hours ago

@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.

edemaine commented 7 hours ago

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?