add exports field to package.json #659

bschlenk · 2024-04-16T20:51:10Z

fixes #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.

bschlenk · 2024-04-16T20:52:40Z

package.json

@@ -8,6 +8,21 @@
  "main": "dist/a11y-dialog.js",
  "module": "dist/a11y-dialog.esm.js",
  "types": "dist/a11y-dialog.d.ts",
+  "exports": {
+    ".": {


When you do import A11yDialog from 'a11y-dialog' you get ./dist/a11y-dialog.esm.js, and when you do const A11yDialog = require('a11y-dialog) you get ./dist/a11y-dialog.js

bschlenk · 2024-04-16T20:53:29Z

package.json

+    "dist/*.esm.js": "./dist/*.esm.js",
+    "dist/*.js": "./dist/*.js",


If someone is importing using an exact extension, give them what they asked for

bschlenk · 2024-04-16T20:54:10Z

package.json

+    "dist/*": {
+      "import": "./dist/*.esm.js",
+      "require": "./dist/*.js",
+      "types": "./dist/*.d.ts"
+    },


Otherwise if the extension is omitted, use the appropriate extension depending on import or require

bschlenk · 2024-04-16T20:54:26Z

package.json

+      "require": "./dist/*.js",
+      "types": "./dist/*.d.ts"
+    },
+    "package.json": "./package.json"


general best practice to allow importing your package.json

KittyGiraudel · 2024-04-28T13:24:05Z

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 · 2024-08-11T07:09:05Z

@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. Like this:

Clone the fork from this PR
Run npm i && npm run build && npm pack
Create a new project, and add this dependency: "a11y-dialog": "file:../a11y-dialog-bschlenk/a11y-dialog-8.0.4.tgz"
Create a .js file, use require('a11y-dialog') and log the result
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 · 2024-08-14T15:25:25Z

👋🏻 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 · 2024-08-14T16:32:18Z

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

KittyGiraudel · 2024-08-14T16:34:09Z

cypress/fixtures/alert-dialog.html

@@ -29,7 +29,7 @@ <h1 id="my-dialog-title">Dialog title</h1>
    </div>
  </div>

-  <script src="./a11y-dialog.js"></script>
+  <script src="./a11y-dialog.cjs"></script>


Is the reason we have to update these local paths because they go strictly through the file system and not at all through the package.json file (therefore ignoring the exports field)?

🤔 I’m actually not sure about that one. Whatever gets tests to pass works 🤷🏻 (it’s been a while since I’ve used Cypress, but if it works, it works)

In general, though, .cjs files should never be in <script> tags, because they won’t be supported in browsers. My guess is that Cypress just doesn’t support ESM, or something.

Actually this has nothing to do with Cypress per se, it’s just a fixture, so I think I could be using the ESM version here. I’ll try.

drwpow-figma · 2024-08-14T17:15:49Z

package.json

@@ -8,6 +8,20 @@
  "main": "dist/a11y-dialog.js",
  "module": "dist/a11y-dialog.esm.js",
  "types": "dist/a11y-dialog.d.ts",
+  "exports": {
+    "import": "./dist/a11y-dialog.esm.js",


Oh this is actually incorrect. The default exports needs to be under ".", which is a special reserved character for the default. In other words, we need to rename exports.import, exports.require, and exports.types to exports['.'].import, exports['.'].require, and exports['.'].types respectively.

Suggested change

"import": "./dist/a11y-dialog.esm.js",

".": {

"import": "./dist/a11y-dialog.esm.js",

"require": "./dist/a11y-dialog.cjs",

"types": "./dist/a11y-dialog.d.ts",

}

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

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.

drwpow-figma · 2024-08-14T17:26:04Z

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 · 2024-08-14T17:30:54Z

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 · 2024-08-14T17:48:01Z

Opened a PR here: #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 · 2024-08-14T18:06:57Z

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

drwpow-figma · 2024-08-14T18:49:13Z

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

add exports field to package.json

7e52522

bschlenk commented Apr 16, 2024

View reviewed changes

KittyGiraudel added 3 commits August 14, 2024 18:29

Output CommonJS files as .cjs with Rollup

a104ab2

Use .cjs files in Cypress fixtures

4f71fba

Update the exports configuration in package.json

644f9b8

KittyGiraudel reviewed Aug 14, 2024

View reviewed changes

KittyGiraudel force-pushed the main branch from bcbc878 to 644f9b8 Compare August 14, 2024 16:36

Merge branch 'main' into main

8eb7233

drwpow-figma reviewed Aug 14, 2024

View reviewed changes

drwpow-figma mentioned this pull request Aug 14, 2024

Build CJS, update package.json #701

Closed

KittyGiraudel mentioned this pull request Aug 14, 2024

Add ‘exports’ field to package.json #702

Merged

KittyGiraudel closed this Aug 14, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add exports field to package.json #659

add exports field to package.json #659

bschlenk commented Apr 16, 2024 •

edited

Loading

bschlenk Apr 16, 2024

bschlenk Apr 16, 2024

bschlenk Apr 16, 2024

bschlenk Apr 16, 2024

KittyGiraudel commented Apr 28, 2024

KittyGiraudel commented Aug 11, 2024 •

edited

Loading

drwpow-figma commented Aug 14, 2024

KittyGiraudel commented Aug 14, 2024 •

edited

Loading

KittyGiraudel Aug 14, 2024

drwpow-figma Aug 14, 2024

drwpow-figma Aug 14, 2024

KittyGiraudel Aug 14, 2024

drwpow-figma Aug 14, 2024

KittyGiraudel Aug 14, 2024

drwpow-figma commented Aug 14, 2024

KittyGiraudel commented Aug 14, 2024 •

edited

Loading

drwpow-figma commented Aug 14, 2024

KittyGiraudel commented Aug 14, 2024

drwpow-figma commented Aug 14, 2024

		"dist/.esm.js": "./dist/.esm.js",
		"dist/.js": "./dist/.js",

add exports field to package.json #659

add exports field to package.json #659

Conversation

bschlenk commented Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KittyGiraudel commented Apr 28, 2024

KittyGiraudel commented Aug 11, 2024 • edited Loading

drwpow-figma commented Aug 14, 2024

KittyGiraudel commented Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drwpow-figma commented Aug 14, 2024

KittyGiraudel commented Aug 14, 2024 • edited Loading

drwpow-figma commented Aug 14, 2024

KittyGiraudel commented Aug 14, 2024

drwpow-figma commented Aug 14, 2024

bschlenk commented Apr 16, 2024 •

edited

Loading

KittyGiraudel commented Aug 11, 2024 •

edited

Loading

KittyGiraudel commented Aug 14, 2024 •

edited

Loading

KittyGiraudel commented Aug 14, 2024 •

edited

Loading