Use .mjs extension for esm files

[IanVS]

What:

This uses .mjs for esm files being exported from the package, since the package itself is not "type": "module". I also marked it as explicitly "type": "commonjs" for good measure.

Also, I added a step to clean out the dist directory before each build.

And combined the dist/esm and dist/cjs directories, since now the files use different extensions.

Why:

https://github.com/testing-library/jest-dom/issues/437#issuecomment-1086939455

How:

Adjusted the rollup config

Checklist:

• [ ] Documentation N/A
• [ ] Tests N/A
• [ ] Updated Type Definitions N/A
• [x] Ready to be merged

I've opened this against the next branch, so that an alpha/beta version can be released.

[codecov[bot]]

Codecov Report

Merging #453 (3edeee3) into next (719b35b) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              next      #453   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        26           
  Lines          624       624           
  Branches       231       231           
=========================================
  Hits           624       624           

Impacted Files	Coverage Δ
src/to-have-form-values.js	100.00% <ø> (ø)
src/to-have-value.js	100.00% <ø> (ø)
src/utils.js	100.00% <ø> (ø)
src/index.js	100.00% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[IanVS]

Hi @gnapse, @sheremet-va, how's this look?

[IanVS]

Any feedback on this?

[IanVS]

It works for me if I add the --experimental-specifier-resolution=node flag.

node --experimental-specifier-resolution=node ./test.mjs

I don't work directly in node.js much, and espeically not with esm, but I do know I've needed to add this flag in the past. Is that not what you normally do?

[sheremet-va]

It works for me if I add the --experimental-specifier-resolution=node flag.

node --experimental-specifier-resolution=node ./test.mjs

I don't work directly in node.js much, and espeically not with esm, but I do know I've needed to add this flag in the past. Is that not what you normally do?

Well, it will be removed when Loader APIs will be finalized. I guess the problem is the other package that doesn't define exports (so, ecosystem problem), or jest-dom can import directly from index.js file.

[IanVS]

Boy, ESM in node is kind of a PITA isn't it? lol.

I added extensions / index.js, and also handled a file that I guess I had missed previously, extend-expect.js. I think using exports like this will work, but I'm not sure if I need the extension in the export map, or not.

[sheremet-va]

Boy, ESM in node is kind of a PITA isn't it? lol.

Yes, ESM<->CJS is a crime against humanity 😢

I think using exports like this will work, but I'm not sure if I need the extension in the export map, or not.

If you want people to import @testing-library/dom/extend-expect, it will not work 👀

[sheremet-va]

I see that everything else is working fine. But I thought you should know:

import '@testing-library/jest-dom/extend-expect' // doesn't work
import '@testing-library/jest-dom/extend-expect.js' // works
const def = require('@testing-library/jest-dom/extend-expect') // doesn't work

[sheremet-va]

I think it might be a good idea to add a test for simply importing jest-dom 😄

[IanVS]

@gnapse @nickmccurdy what is the purpose of /extend-expect.js? If we're talking about making breaking changes for the next major anyway, is there a reason to keep this extra export around, that seems to have just re-exported the main index file anyway?

[nickmccurdy]

I assume it's just there for back compatibility. I agree that we probably don't need to keep it around for the next major (as long as we move the one-line implementation back to index.js).

[IanVS]

@nickmccurdy, done, thanks. I believe this is ready for review.

[IanVS]

@nickmccurdy @gnapse Hi, what are your thoughts on merging this and cutting a beta soon?

[IanVS]

Hi, friendly ping that I think this is ready to go. :)

[IanVS]

Hi, it's a bit of a shame that a breaking change was released without this PR and the related change (https://github.com/testing-library/jest-dom/pull/438) to build and publish ESM. Was there a reason not to include them? Is another breaking change on the horizon? @nickmccurdy @gnapse @kentcdodds. 🙏

[kentcdodds]

Sorry, there's a skeleton crew maintaining this project. That's why this wasn't merged. I'm afraid I only have the bandwidth to work on things that directly impact my own work.

[IanVS]

OK, thanks for responding. I only pinged you because I saw you were involved recently in the latest breaking release, and I'm a bit surprised that you're not using jest-dom and/or esm in your latest work. At any rate, I'm creating a new PR on top of the latest changes in main, to hopefully make it easier for current maintainers to review/merge.

[kentcdodds]

I am actually, but with vitest and I don't think I've had issues. I'm going to look at it again in the future and if this fixes things for me then I may merge your other PR unless another maintainer steps up before I get to it

[jgoz]

@IanVS I think dual-publishing ESM/CJS could (and should?) be done in a non-breaking way, especially now that we've already done one breaking change that is creating some confusion amongst users (😅).

[IanVS]

Thanks, @jgoz I agree after taking another look at it. This PR was removing extend-expect, but you already did that in #511. 🙌

I've opened #519 as another attempt to dual-publish, with the latest 6.0 changes you made.

[nickmccurdy]

Shouldn't this be reopened so #519 can close it? I'm not seeing .mjs files in Jest DOM 6.

[IanVS]

@nickmccurdy This isn't an issue, it's a PR that I've decided to abandon and re-implement in #519. It can be re-opened if you'd like, but I don't think it should be reviewed/merged, so I was trying to keep things clean.

[nickmccurdy]

You're right, I got mixed up when reviewing other notifications here.