add exports field to package.json

[bschlenk]

fixes https://github.com/KittyGiraudel/a11y-dialog/issues/646

Left some inline comments on what each part is for. Mostly I'm being defensive, so that if any of these import patterns are already in use, they'll still work and this can be a minor version bump.

I'd recommend at some point locking this down to just the . and the package.json entries and making that a major version bump.

[KittyGiraudel]

Thank you so much for the PR and the explanations! Do you think we should do that in v9 or is it safe to ship as is? Is there any import that could potentially break when setting this up?

[KittyGiraudel]

@bschlenk Hello! 👋

I’m finally getting back to this PR after so long.

Note to myself, this is how I tested it.

I followed the `npm pack` strategy from [this guide](https://urre.me/writings/test-local-npm-packages/). Like this: 1. Clone the fork from this PR 2. Run `npm i && npm run build && npm pack` 3. Create a new project, and add this dependency: `"a11y-dialog": "file:../a11y-dialog-bschlenk/a11y-dialog-8.0.4.tgz"` 4. Create a `.js` file, use `require('a11y-dialog')` and log the result 5. Create a `.mjs` file, use `import A11yDialog from 'a11y-dialog'` and log the result

I tried it as is, and this is the error I got (both using require in a .js file and using import in a .mjs file):

Error [ERR_INVALID_PACKAGE_CONFIG]: Invalid package config /Users/kitty/Sites/a11y-dialog-bschlenk-test/node_modules/a11y-dialog/package.json. "exports" cannot contain some keys starting with '.' and some not. The exports object must either be an object of package subpath keys or an object of main entry condition name keys only.

Following the content of the error, I tried updating the configuration to look like this (basically just removing the "." layer):

"exports": {
    "import": "./dist/a11y-dialog.esm.js",
    "require": "./dist/a11y-dialog.js",
    "types": "./dist/a11y-dialog.d.ts",
    "dist/*.esm.js": "./dist/*.esm.js",
    "dist/*.js": "./dist/*.js",
    "dist/*": {
      "import": "./dist/*.esm.js",
      "require": "./dist/*.js",
      "types": "./dist/*.d.ts"
    },
    "package.json": "./package.json"
  },

This did fix the case of using import in a .mjs file (yay!), but when using require in a .js file, I got the following error:

Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/kitty/Sites/a11y-dialog-bschlenk-test/node_modules/a11y-dialog/dist/a11y-dialog.js from /Users/kitty/Sites/a11y-dialog-bschlenk-test/index.js not supported. a11y-dialog.js is treated as an ES module file as it is a .js file whose nearest parent package.json contains "type": "module" which declares all .js files in that package scope as ES modules. Instead either rename a11y-dialog.js to end in .cjs, change the requiring code to use dynamic import() which is available in all CommonJS modules, or change "type": "module" to "type": "commonjs" in /Users/kitty/Sites/a11y-dialog-bschlenk-test/node_modules/a11y-dialog/package.json to treat all .js files as CommonJS (using .mjs for all ES modules instead).

My understanding is that we should either remove "type": "module" from the package.json, but I don’t know if we want that…? Or we should spit out .cjs files when building CJS/UMD, instead of .js files. We could do that, but then that would be a breaking change, right?

[drwpow-figma]

👋🏻 Hi! (I work with @bschlenk and he’s out for a while)

My understanding is that we should either remove "type": "module" from the package.json, but I don’t know if we want that…?

Right it’s better to keep "type": "module" in packages. This is the “new” way that works universally in browsers and Node. Not having this keeps it in “legacy” CommonJS mode. The only reason Node hasn’t made "type": "module" the default is it’s taking a lot of time for legacy packages to update.

Or we should spit out .cjs files when building CJS/UMD, instead of .js files. We could do that, but then that would be a breaking change, right?

It won’t necessarily be a breaking change! As long as it’s set up correctly. But yes all CommonJS output should be in .cjs files because this package is in “ESM mode” ("type": "module"). However, having TS definition files in the mix makes it slightly-annoying—.cjs files have to have matching *.d.cts files (not *.d.ts).

Whether ESM and CJS lives in the same folder or separate folders is a matter of preference, and doesn’t matter.

How we prevent breaking changes is what @bschlenk added in his comment above—you can actually remap exact filepaths to different files in the dist folder with the exports object. So you can keep all the old paths for backwards compatibility, while ensuring they work for ESM and CJS (you can think of them like “aliases,” basically—to a user they will work exactly the same, but underneath the hood you can rename the package contents).

[KittyGiraudel]

@drwpow-figma Hello and thank you very much for getting back to me with some very valuable information! I issued a few commits to update the pull-request based on the information I understood. Would you mind doing a code review to see if I didn’t mess up anything? It’s mostly important that we don’t cause any breaking change. 😅

[drwpow-figma]

Would you mind doing a code review to see if I didn’t mess up anything? It’s mostly important that we don’t cause any breaking change. 😅

Oh so on closer inspection, renaming UMD files to .cjs is bad, and we’d be breaking users that are using UNPKG, since package.json is ignored completely by browsers. As a general rule:

• ESM: should be .js (since this is an ESM package with type: "module")
• CJS: should be .cjs
• UMD: this could actually be named either .js or .mjs (but definitely NOT .cjs, because browsers will reject that)

Sorry I didn’t catch this earlier; I was still orienting myself to the package. So your question about the Cypress changes was in fact spot-on: those shouldn’t have changed. Let me spend a little time on an improvement for what you’ve done to fix the issues in a backwards-compatible way (this is tricky because I don’t think the tests comprehensively test all the scenarios that could break for people).

[KittyGiraudel]

What we can also do if that’s easier is to assume this change cannot safely be done in a minor, and plan it for v9. And therefore come up with the state-of-the-art setup for all cases.

[drwpow-figma]

Opened a PR here: https://github.com/KittyGiraudel/a11y-dialog/pull/701 because I don’t know how to open a PR against this one. But you get the gist (you can just cherry-pick the commit; I don’t care about attribution).

In my PR, I leave the .js files as UMD so we don’t break backwards-compat with existing installations. But I added building true .cjs files so that Node is happy, too. This should be a backwards-compatible upgrade, where newer versions of Node get served better files. And everyone already using this library still consumes it 100% the same way.

As I mentioned in one of my messages, it fails if we use both "." and defined paths:

Ah right—you know that syntax may be valid too now that I think of it; I’m just far more used to the dot-preceder. I followed that syntax in my PR, but it may just be personal preference (may not matter).

[KittyGiraudel]

Closing in favor of #702. Thank you for the super insightful comments here!

[drwpow-figma]

What we can also do if that’s easier is to assume this change cannot safely be done in a minor, and plan it for v9.

Oh also just FYI: in package.json, module and types are deprecated (module was technically never supported by Node.js; it was just a webpack-ism that spread). Well, types isn’t technically deprecated, but TS now recommends just having types inside exports since that will take priority if using that (and all current versions of Node now support this, and probably most people are on recent-enough versions of TS that support this too, so types is probably just being ignored most of the time by users but it can be kept for backwards compat; see docs).