KittyGiraudel / a11y-dialog

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

Add ‘exports’ field to package.json #702

Closed KittyGiraudel closed 1 month ago

KittyGiraudel commented 1 month ago

Cherry-picked from both #659 and #701. Refer to the 2 other pull-requests for discussion and comments.

KittyGiraudel commented 1 month ago

@drwpow-figma Do you have some suggestions on how to test (like automatically I mean) that sort of things? Like import, CJS and stuff?

drwpow-figma commented 1 month ago

So yeah it is possible to test this! But it would require a little setup. Cypress is testing the UMD version, so that one’s covered. But it’s the ESM + new CJS versions we want to add tests for.

I think you could take 1 of 2 strategies (maybe both)

Option 1. Automated tests

This one only tests that the code imports correctly.

  1. Make 2 sub-packages in this repo (e.g. test/fixtures/cjs/package.json and test/fixtures/esm/package.json).
  2. These must have package.jsons set to "type": "commonjs" and "type": "module" respectively.
  3. In both package.jsons, add a dependency with "a11y-dialog": "file:../../" (unless you move to pnpm workspaces, where this setup would be more automatic).
  4. Add a "test": "node test.js" script that imports the a11y-dialog package and makes a basic assertion, e.g.:
// text/fixtures/cjs/test.js
const a11yDialog = require('a11y-dialog');
const assert = require('node:assert');

assert(a11yDialog !== undefined); // this will have already thrown an error if something went wrong
// test/fixtures/esm/test.js
import a11yDialog from 'a11y-dialog';
import assert from 'node:assert';

assert(a11yDialog !== undefined); // this will have already thrown an error if something went wrong

That’s the gist of it. The annoying part is you will have to run npm i in both sub-packages if you don’t switch this package to pnpm workspaces, in which case a single pnpm i in the root will install everything (npm also has its own version of workspaces, which this repo is simple enough it might work, but for larger projects there are too many papercuts I strongly recommend pnpm). Further, you’ll also have to cd [dir] && npm test for each test, again, unless you switch to pnpm where you can run these together ("test": "pnpm run --recursive \"test\"").

The downside of the automated testing is you’re not actually testing runtime, only imports but that’s probably OK because the Cypress test covers this?

Option 2. Manual Examples

Alternately, you still would need 2 subpackages with "type": "commonjs" and "type": "module" respectively. But these would be more working examples that run in a browser, through a framework.

  1. CJS: Uses the NextJS starter (npx create-next-app@latest) (docs)
  2. ESM: Uses the Vite + React starter (npx create vite@latest) (docs)

Worth noting that Next.js transforms everything through webpack, which is still CJS at its core (it downconverts ESM to CJS). Vite, likewise, requires ESM to run (it upconverts CJS to ESM). In both examples, you’d be using React, yes, which is unrelated to this library. But the examples would ensure they do work inside a more common setup than just a vanilla JS app manipulating the DOM directly (you’d also get to test out how this works inside Next.js SSR too).

The beauty of this approach is you test everything—import and runtime, and is accurate and reliable. Plus, you also get some docs benefits from it, where users have more examples on how to use this code. The downside, of course, is these have to be run manually. But consider how rare it is someone will modify the package.json exports—like once a year, if that?—so automated tests may just be a waste of time and compute in the 99.9% of PRs that don’t need to test the imports.

Option 3. Automated tests, but make it even more annoying

You may think “but wait—we could have automated JSDOM tests!” And you could… except you’d still have to have the 2 sub-packages. AND you’d have to use different test runners for both. Jest (still) only supports CJS (you have to manually downconvert ESM back to CJS via Babel; it also has experimental ESM support but that’s still not stable yet). Vitest is an improvement over Jest in every way, but only supports ESM. The only real assurance you’d have that you were testing what you thought was to use one testrunner in one package, and the other in the other.

From there you could do more standard JSDOM tests that would be reliable enough to bank on IMO. But now you’re stuck with a Cypress + Jest + Vitest testing combo that won’t be fun to maintain.

KittyGiraudel commented 1 month ago

Wow, thank you so much for writing such a comprehensive answer! 💖

Option 1 is essentially what I was looking for. I just want to make sure things work in all formats (as in can be safely imported with an ESM error or something). So this approach seems totally fair. The library behavior + browser integration is covered by our Cypress tests (quite well I would say).

I need to investigate the npm workspaces to see if we can avoid using pnpm/yarn; if we can’t, maybe bite the bullet and make the switch but ideally not.

drwpow-figma commented 1 month ago

Option 1 is essentially what I was looking for. I just want to make sure things work in all formats (as in can be safely imported with an ESM error or something). So this approach seems totally fair.

Awesome. I agree—I think that does what you want relatively cheaply, and is reliable!

I need to investigate the npm workspaces to see if we can avoid using pnpm/yarn; if we can’t, maybe bite the bullet and make the switch but ideally not.

I cannot recommend pnpm enough. I feel your hesitation as I didn’t want to switch, having a repeat of yarn. But I’d argue pnpm has overcome npm in the default package manager for libraries and apps. It works so well because it matches npm’s design 1:1—nothing to learn (unlike Yarn), everything works as-expected (you just add a p). But what you get in return is better… everything. Faster installs, dramatically sped-up CI times, better workspaces. Every platform supports it well (including GitHub Actions). And pnpm run has so many QoL improvements such as deterministic build order execution (in a monorepo, it will build packages that rely on other packages in the monorepo in the order they depend on each other, so you won’t get missing package errors). I’ve been using pnpm for 3+ years now and am never looking back.

No pressure though—it’s your call! 🙂 But it’s such a smoother experience than Yarn was, or dare I say npm!