Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ‘exports’ field to package.json #702

Merged
merged 2 commits into from
Aug 16, 2024
Merged

Conversation

KittyGiraudel
Copy link
Owner

@KittyGiraudel KittyGiraudel commented Aug 14, 2024

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

KittyGiraudel and others added 2 commits August 14, 2024 20:03
Co-authored-by: Drew Powers <apowers@figma.com>
Co-authored-by: Brian Schlenker <bschlenker@figma.com>
Co-authored-by: Drew Powers <apowers@figma.com>
Co-authored-by: Brian Schlenker <bschlenker@figma.com>
Copy link
Contributor

@drwpow-figma drwpow-figma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! :shipit:

@KittyGiraudel
Copy link
Owner Author

@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
Copy link
Contributor

drwpow-figma commented Aug 15, 2024

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
Copy link
Owner Author

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
Copy link
Contributor

drwpow-figma commented Aug 15, 2024

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!

@KittyGiraudel KittyGiraudel merged commit 4770d02 into main Aug 16, 2024
3 checks passed
@KittyGiraudel KittyGiraudel deleted the package-exports-field branch August 16, 2024 05:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants