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

Use .mjs extension for esm files #453

Closed
wants to merge 6 commits into from

Conversation

IanVS
Copy link
Contributor

@IanVS IanVS commented Apr 28, 2022

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:

#437 (comment)

How:

Adjusted the rollup config

Checklist:

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

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

@codecov
Copy link

codecov bot commented Apr 28, 2022

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%> (ø)

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

@IanVS
Copy link
Contributor Author

IanVS commented May 20, 2022

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

@IanVS
Copy link
Contributor Author

IanVS commented Aug 11, 2022

Any feedback on this?

rollup.config.js Outdated
index: 'src/index.js',
matchers: 'src/matchers.js',
},
input: 'src/index.js',

Choose a reason for hiding this comment

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

Maybe it's better to provide an array for an input? So rollup can share code between files, otherwise matchers are duplicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not terribly familiar with rollup. How can I specify an array for input, when each one should have a specific different output?

Copy link

@sheremet-va sheremet-va Aug 11, 2022

Choose a reason for hiding this comment

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

There is an example in Vitest repo: https://github.com/vitest-dev/vitest/blob/main/packages/vitest/rollup.config.js

You will need two entries, something like this:

export default [
	  {
	    input: entries,
	    output: {
	      dir: 'dist',
	      entryFileNames: '[name].mjs',
	      format: 'esm'
	    }
	  },
	  {
	    input: entries,
	    output: {
	      dir: 'dist',
	      entryFileNames: '[name].js',
	      format: 'cjs'
	    }
	  }
]

@IanVS IanVS mentioned this pull request Aug 31, 2022
2 tasks
Copy link

@sheremet-va sheremet-va left a comment

Choose a reason for hiding this comment

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

Rollup now produces .js file with ESM content alongside correct .mjs file 😞

This file should be generated, so you don't have double code for /index and /matchers entry points.

I've added two possible solutions.

input: entries,
output: {
dir: 'dist',
entryFileNames: '[name].mjs',

Choose a reason for hiding this comment

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

You can add chunkFileNames: '[name]-[hash].mjs', so chunks will also have correct filepath.

You can also add preserveModules: true, then rollup will not generate "chunks", it will have the same file structure as your source code.

I guess the first solution is closest to how it was, so 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good eye, thanks for catching that. I've taken your first suggestion, and mirrored it on the .js as well, just for consistency / to be explicit.

Copy link

@sheremet-va sheremet-va left a comment

Choose a reason for hiding this comment

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

I've tried this file, and it unfortunately fails with folder error :(

// test.mjs
import {toBeChecked} from './dist/matchers.mjs'

console.log(toBeChecked)

@IanVS
Copy link
Contributor Author

IanVS commented Sep 19, 2022

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

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.

This was previously handled as a file in the root of the project that redirected to the built index file.
This change removes a layer of indirection by removing src/extend-expect, and using
exports to point to the index file rather than using a real file in the project root.
@IanVS
Copy link
Contributor Author

IanVS commented Sep 19, 2022

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

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

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

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

@IanVS
Copy link
Contributor Author

IanVS commented Sep 19, 2022

@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?

@nickserv
Copy link
Member

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

IanVS commented Sep 22, 2022

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

Copy link

@sheremet-va sheremet-va left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

@IanVS
Copy link
Contributor Author

IanVS commented Oct 6, 2022

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

@IanVS
Copy link
Contributor Author

IanVS commented Oct 26, 2022

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

@IanVS
Copy link
Contributor Author

IanVS commented Aug 22, 2023

Hi, it's a bit of a shame that a breaking change was released without this PR and the related change (#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
Copy link
Member

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

IanVS commented Aug 22, 2023

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.

@IanVS IanVS closed this Aug 22, 2023
@kentcdodds
Copy link
Member

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

jgoz commented Aug 22, 2023

@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 IanVS mentioned this pull request Aug 22, 2023
4 tasks
@IanVS
Copy link
Contributor Author

IanVS commented Aug 22, 2023

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.

@nickserv
Copy link
Member

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

@IanVS
Copy link
Contributor Author

IanVS commented Aug 22, 2023

@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.

@nickserv
Copy link
Member

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

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.

5 participants