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

Publish ES modules #437

Closed
IanVS opened this issue Feb 24, 2022 · 11 comments · Fixed by #438 or #519
Closed

Publish ES modules #437

IanVS opened this issue Feb 24, 2022 · 11 comments · Fixed by #438 or #519
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed released

Comments

@IanVS
Copy link
Contributor

IanVS commented Feb 24, 2022

Describe the feature you'd like:

I am a maintainer of storybook-builder-vite, which allows storybook users to build their previews using vite, which works entirely in ESM. Storybook recently also released @storybook/jest and the latest release extends expect with jest-dom matchers. However, since jest-dom is only published as commonjs, typescript needs to add a compatibility layer so that import * as matchers from @testing-library/jest-dom/matchers; can be used. This is causing an issue, whereby a default is being included as well, causing expect to throw an error. See storybookjs/jest#15 (comment) for the error description.

To avoid the esm/cjs bundling problems, I'd like to request that jest-dom publish an esm version alongside cjs.

Suggested implementation:

I see that you're using kcd-scripts build, which also allows a --bundle flag that builds cjs, esm, umd, and umd.min bundles. Alternatively, https://github.com/unjs/unbuild is a very simple build tool which can generate multiple targets.

Describe alternatives you've considered:

I'm trying to troubleshoot the symptom of the esm->cjs namespace import to see if there's a way I can work around it in the storybook packages, but I think it would be nice if this package could publish esm anyway, especially now that jest itself is publishing es modules.

Teachability, Documentation, Adoption, Migration Strategy:

I think as long as the package.json is updated correctly, there shouldn't be any user-facing changes needed. That said, bundling the files as suggested above will cause problems if users are importing files directly from node_modules/@testing-library/jest-dom/dist. If that's the case, and it's a use case that needs to be supported, then a different build process will be needed, but can still be accomplished.

@gnapse gnapse added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed labels Feb 24, 2022
@bodograumann
Copy link

Would it make sense to explicitely expose the matchers as well?
Seems to me like we are relying on private implementation details, when we import * as matchers from '@testing-library/jest-dom/matchers'.

(Btw. did you mean to write the import the way you did above, @IanVS ?)

@IanVS
Copy link
Contributor Author

IanVS commented Feb 24, 2022

(Btw. did you mean to write the import the way you did above, @IanVS ?)

Nope, thanks, I fixed it now.

Furthermore, it seems this is not a typescript issue as I first expected, but due to the behavior of ESBuild (used by vite). I see that @gnapse gave this a 👍, so I'll try to put together a PR.

@IanVS
Copy link
Contributor Author

IanVS commented Feb 24, 2022

@gnapse does each of the custom matchers need to be exported individually, or just the bundle of them all together matchers.js?

@IanVS IanVS mentioned this issue Feb 24, 2022
4 tasks
@gnapse
Copy link
Member

gnapse commented Feb 25, 2022

I do not think we've ever supported or needed to support having them consumed/imported individually. Bundling them all together in matchers.js is enough.

@weyert
Copy link

weyert commented Feb 25, 2022

I would like to say thank you for still supporting common js and not force a ESM only upgrade :)

@IanVS
Copy link
Contributor Author

IanVS commented Feb 25, 2022

Bundling them all together in matchers.js is enough.

Great, that's what I did in #438. 👍

not force a ESM only upgrade

I assume this is mostly a nodejs tool (although I use it in the browser), I don't think it would make sense to forsake cjs for a long time to come.

@sheremet-va
Copy link

sheremet-va commented Apr 3, 2022

Is it possible to store esm files with mjs extension? Node actually doesn’t allow dual packaging, and importing file will fail with tools like Vitest, that rely on exports field to find full path and import it.

Bundlers don’t run actual code, only read it, so it works with most frontend usecases, but doesn’t run in actual Node runner.

You can see example here: https://github.com/sheremet-va/dual-packaging

@IanVS
Copy link
Contributor Author

IanVS commented Apr 3, 2022

It seems like you're more familiar with what is needed than I am, maybe you could submit a PR? Is it just a matter of changing the extension in the rollup config?

@sheremet-va
Copy link

It seems like you're more familiar with what is needed than I am, maybe you could submit a PR? Is it just a matter of changing the extension in the rollup config?

Yeah, extension should be .mjs. That's it 😄

@IanVS
Copy link
Contributor Author

IanVS commented Apr 28, 2022

@sheremet-va I was trying this out using your helpful guide, but I'm not quite sure how to handle having two separate inputs. Right now, I'm providing a dir as the output, and I guess rollup itself is adding the .js extension. Have you dealt with this before?

Edit: Hmmm, I think I can maybe do this by returning more than one top-level object. I guess I asked for help too soon.

@github-actions
Copy link

🎉 This issue has been resolved in version 6.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants