search

react-dropzone / attr-accept

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

chore: publish ESM only #66

Open remcohaszing opened 1 week ago

remcohaszing 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 This change removes the build process of compiling the package to CJS, making the packages ESM only. Since ESM is only supported in more modern versions of Node.js, the minimal Node.js version requirement was increased to 18. This is reflected in the package.json engines field and GitHub action workflows.

The build process of downleveling the ESM code was also removed, as modern versions of Node.js support all syntax used. As a result, all build related dependencies could be removed.

There were some issues running the tests with the old version of Mocha used. Instead of updating Mocha, I replaced it with the builtin Node.js test runner.

Since new versions have issues consuming the old version 1 lockfile, it was deleted and a new version 3 lockfile was generated.

Does this PR introduce a breaking change? Yes. The CJS build is no longer available

Other information This change was discussed in https://github.com/react-dropzone/attr-accept/pull/65#issuecomment-2367631383

A potential follow-up PR could generate the type definitions from the JSdoc annotations and type check everything.

Closes #29 Closes #63 Closes #64

rolandjitsu commented 1 week ago

@remcohaszing what happens to old clients that haven't upgraded yet to node 18+? E.g. this pkg is used in react-dropzone, and so, if we have some clients using older versions of node, will this pkg still be consumable (as far as I can tell, it should be ok)?

Also, this would be a breaking change, so the commit must be a feat and include a BREAKING CHANGE (see https://www.conventionalcommits.org/en/v1.0.0/).

remcohaszing commented 1 week ago

I picked Node.js 18 because that’s the latest Node.js version that’s not EOL. Of course that doesn’t mean you may not support older versions. Node.js 12 was the first version to add ESM support. This package doesn’t use really fancy modern JavaScript features, so that should just work.

There things that determine which Node.js version is supported are:

rolandjitsu commented 1 week ago

@remcohaszing I'm all up for modernisation.

The only caveat here is that users on old versions of node will have to upgrade to node 18+ if they want new features (assuming we'll land some in the future).

That means that react-dropzone will stick to the current version as it's engine is set to 10+ to maintain compatibility (actually, node doesn't really matter at runtime where react-dropzone is used).

Which is why I asked the question. Ideally, we should maintain compatibility with older versions, but if that's too difficult, we'll go ahead with a breaking change and bump the major.

edemaine commented 1 week ago

To me a bigger issue with moving to ESM only is that you cannot require ESM modules from CJS code. I thought this would be true in all versions of NodeJS, but it's apparently changing in the future.

I don't have a particular need for a CJS build of this package, but maybe someone else does? Anyway, up to the maintainer what's best here; just wanted to provide some context so you can make an informed decision.

rolandjitsu commented 1 week ago

@remcohaszing do you think that we could, instead of completely dropping cjs support, just add support for esm in this PR? Or would that require too much work? We're currently trying that for react-dropzone.

I would definitely transition all dev deps to latest and get rid of mocha, but I'd really like to keep supporting older clients for now and wait a bit more before migrating to pure esm.

Otherwise, any fix would need to also go into older versions (a v 2.2.x maintenance branch).

edemaine commented 1 week ago

do you think that we could, instead of completely dropping cjs support, just add support for esm in this PR?

I guess like #64? Honestly, I don't even remember writing #64, and it seems to have an outstanding comment, but #64 looks pretty simple and so would the comment fix. I could go back to revising it if that would be helpful. But maybe there have been various other changes in the meantime. This PR (#66) also does other things, like switch from Mocha to Node's new testing harness.

rolandjitsu commented 1 week ago

@edemaine it's up to you. @remcohaszing what are your thoughts?

As for the other changes, I would pull that out into a separate PR.

edemaine commented 1 week ago

I've gone ahead and revised #64, which had minimal conflicts (only README) so I don't think it's outdated. If it's considered good, perhaps the other features of this PR could be built on top of that one? I just read #65 and see the original intent was to fix the CJS exports. That definitely seems worth doing, and isn't in #64.

Alternatively, if it's easier to redo this PR with dual CJS/ESM, that would seem like an improvement to me.

rolandjitsu commented 1 week ago

I've gone ahead and revised #64, which had minimal conflicts (only README) so I don't think it's outdated. If it's considered good, perhaps the other features of this PR could be built on top of that one? I just read #65 and see the original intent was to fix the CJS exports. That definitely seems worth doing, and isn't in #64.

Alternatively, if it's easier to redo this PR with dual CJS/ESM, that would seem like an improvement to me.

Thanks @edemaine . I'm trying to work on some changes locally as well to see what can be done. Note that in your PR, if you bump versions of pkgs, the lock file needs to be regenerated.

I'm just noticing now that there are a ton of dependencies that are out of date 😬 I've started bumping things, but some haven't been updated in years either ... so unless I go ahead and updated those, we'll have to stick with current versions.

There are also the issues around the package.lock that @remcohaszing mentioned ... I'm on node 20 and it's taking forever to install deps via npm with the current lock file, removing it seems to do the trick. But that probably means we'll need users to be on later versions of node.

We may end up dropping support for older node versions and going ahead with esm. Older clients can probably use some transpilers to be able to consume esm if they cannot yet.

edemaine commented 1 week ago

I've also failed to successfully run npm install (seems to hang forever). Thanks for the tip to delete the lock file!

Like I said, I'm not worried about supporting old Node versions. The issue is that modern Node versions still support CJS (and I doubt that will ever disappear), and packages that haven't migrated to ESM yet won't be able to use this anymore. I guess there's a trend to force people to update to ESM by publishing ESM only, but I'm not a fan personally. Of course, it's entirely up to you what you prefer!

rolandjitsu commented 1 week ago

@remcohaszing and @edemaine I've opened https://github.com/react-dropzone/attr-accept/pull/75 where we do most of what's being done here and also ensures some of the other things that worked before still work (e.g. size limit check).

While I would have preferred to keep supporting CommonJS, in the long run, it makes more sense to migrate to ESM.