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

chore: migrate pretty-format to ESM #10515

Merged
merged 12 commits into from
Nov 26, 2020
Merged

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Sep 14, 2020

Summary

Changed export from a "default" (CJS) export to a named function called format.

We can remove the patch when making a new release (as at that point the version range won't match @testing-library/dom and it'll pull in a compatible version)

Test plan

Green CI

@SimenB SimenB added this to the Jest 27 milestone Sep 14, 2020
@SimenB SimenB force-pushed the pretty-format-esm branch 2 times, most recently from 5a321d0 to 0e91708 Compare September 15, 2020 05:48
@thymikee
Copy link
Collaborator

thymikee commented Sep 15, 2020

Why not have format as both named and default ESM export? This way we would break the world, at least for those that don't use require("pretty-format") directly.

@SimenB
Copy link
Member Author

SimenB commented Sep 15, 2020

can do a default export, sure. Just as breaking for CJS peeps, but no biggie.

Do you want both named and default, or just default?

@SimenB
Copy link
Member Author

SimenB commented Sep 15, 2020

Note that today people do

import prettyFormat from 'pretty-format';

const plugins = prettyFormat.plugins;

That I wanna break regardless. It's so weird that doing this uncovered a bug in tsc 😛 (microsoft/TypeScript#29206)

@thymikee
Copy link
Collaborator

I'd go with both default and named.

today people do
import prettyFormat from 'pretty-format';
const plugins = prettyFormat.plugins;

I don't mind breaking this use case, it's wrong usage of ESM and should be fixed anyway.

@SimenB
Copy link
Member Author

SimenB commented Sep 15, 2020

Cool! 👍 I'll make the change before landing. No timeline on 27, so I'll just leave this as is for now (unless I get bored (again))

```

```js
import prettyFormat from 'pretty-format'; // ES2015 modules
import {format as prettyFormat} from 'pretty-format'; // ES2015 modules
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm purposefully not using default import here, since real ESM only works with named exports (in node impl) as it's still CJS under the hood

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd go with just format in examples, without aliasing to prettyFormat. No need for extra mental overhead here

Copy link
Member Author

Choose a reason for hiding this comment

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

it's the difference between "copy the example and run it" vs "copy example, but get weird errors". I think it's worth it to keep as is?

@SimenB
Copy link
Member Author

SimenB commented Nov 4, 2020

@thymikee thoughts on it now?

import prettyFormat = require('pretty-format');
import {
Plugin as PrettyFormatPlugin,
Plugins as PrettyFormatPlugins,
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we use import type here?

Copy link
Member Author

Choose a reason for hiding this comment

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

we import plugins below, so no

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

👍

@SimenB SimenB merged commit 034cd34 into jestjs:master Nov 26, 2020
@SimenB SimenB deleted the pretty-format-esm branch November 26, 2020 18:55
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants