KittyGiraudel / a11y-dialog

A very lightweight and flexible accessible modal dialog script.
https://a11y-dialog.netlify.app
MIT License
2.39k stars 132 forks source link

Package.json has type: module but main still points at commonjs #646

Closed bschlenk closed 4 days ago

bschlenk commented 5 months ago

I just started using this library and it works great! Thanks for your efforts here.

Now I'm writing tests (and running with vitest), but it seems to be picking up the commonjs version of the build. This results in the error "default is not a constructor".

I'm not 100% sure, but it might be that vitest is following closely with the module spec, where the module field of package.json isn't actually used, instead it uses the main field when there isn't an exports field. The module field of package.json isn't standard, so it makes sense that vitest wouldn't use it.

As a sanity check, I changed the main field to point to the .esm.js file and stopped getting that error. I wouldn't recommend that for production though, I think this should be updated to use the exports field and use conditional exports for require and import.

bschlenk commented 5 months ago

Here's a related discussion in the vitest repo https://github.com/vitest-dev/vitest/discussions/4233

bschlenk commented 5 months ago

If anyone else runs into this, I worked around this by adding an alias to my vite config:

alias: {
  'a11y-dialog': path.resolve(__dirname, './node_modules/a11y-dialog/dist/a11y-dialog.esm.js'),
},
KittyGiraudel commented 5 months ago

Hello Brian, and thanks for opening an issue. ๐Ÿ‘‹

I must say this is the part of maintaining a package I am really unexperienced with, and I am always terrified of breaking things by touching the entry points or the bundling system. I recall having to release several hotfixes after v8 to have something that works. ๐Ÿ˜“

If you are pretty confident with the exports approach, you could consider opening a MR and Iโ€™ll happily review it. Otherwise, maybe we can just add your workaround to the docs. What do you think?

bschlenk commented 5 months ago

Yeah I agree touching exports can be scary!

I'm happy to send a MR when I get a chance. I think the biggest risk is that a user might be reaching into the package to grab some internals, and the exports change makes those internals no longer public. So the safest bet is to make sure that all files that are currently in the dist folder remain accessible. You can always lock that down further in a major version update, but this would be the safest bet to keep it a minor version bump.

KittyGiraudel commented 1 week ago

Fixed, but it needs to be released.

KittyGiraudel commented 4 days ago

Done in version 8.1.0. ๐ŸŽ‰ Thank you so much for your support! I hope we havenโ€˜t broken anything, but Iโ€™m pretty confident. ๐Ÿ˜