-
Notifications
You must be signed in to change notification settings - Fork 130
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
Conversation
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
@drwpow-figma Do you have some suggestions on how to test (like automatically I mean) that sort of things? Like import, CJS and stuff? |
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 testsThis one only tests that the code imports correctly.
// 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 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 ExamplesAlternately, you still would need 2 subpackages with
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 Option 3. Automated tests, but make it even more annoyingYou 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. |
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. |
Awesome. I agree—I think that does what you want relatively cheaply, and is reliable!
I cannot recommend No pressure though—it’s your call! 🙂 But it’s such a smoother experience than Yarn was, or dare I say npm! |
Cherry-picked from both #659 and #701. Refer to the 2 other pull-requests for discussion and comments.