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

Support Prettier 3 #14566

Merged
merged 9 commits into from
Sep 25, 2023
Merged

Support Prettier 3 #14566

merged 9 commits into from
Sep 25, 2023

Conversation

liuxingbaoyu
Copy link
Contributor

@liuxingbaoyu liuxingbaoyu commented Sep 24, 2023

Summary

Support Prettier 3
Fixes: #14305
Closes #14311

Test plan

CI green
I tried adding tests, but I failed when importing ESM's prettier.
In addition, the import in worker_threads may be complicated.

@netlify
Copy link

netlify bot commented Sep 24, 2023

Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit b78f15a
🔍 Latest deploy log https://app.netlify.com/sites/jestjs/deploys/65114041ffe9460008eaa1c0
😎 Deploy Preview https://deploy-preview-14566--jestjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@SimenB
Copy link
Member

SimenB commented Sep 24, 2023

Interesting! CI is unhappy, but exciting nonetheless 😃

This does essentially what https://github.com/prettier/prettier-synchronized does, right? I tried to use it in #14311, but hit prettier/prettier-synchronized#4 (comment)

@liuxingbaoyu
Copy link
Contributor Author

liuxingbaoyu commented Sep 24, 2023

This does essentially what prettier/prettier-synchronized does, right? I tried to use it in #14311, but hit prettier/prettier-synchronized#4 (comment)

Yes!

(Except for a little non-public API🤦‍♂️)
I have tried many methods, but it is difficult to modify the prettier AST elegantly. So I'm using private API now.

cc @fisker Do you have any suggestions?

Also I found out that the E2E tests are using Prettier 3, which means as long as the CI is green, everything should be fine.

@fisker
Copy link
Contributor

fisker commented Sep 24, 2023

Do you have any suggestions?

Since we removed custom parser support in v3, yes, it's difficult to modify ast. I don't have a better idea.

writeFiles(DIR, {
'jest.config.js': `
module.exports = {prettierPath: require.resolve('prettier-2')};
module.exports = {prettierPath: require.resolve('prettier')};
Copy link
Contributor

Choose a reason for hiding this comment

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

This test should running against the prettier/index.cjs, maybe better run against prettier/index.mjs too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since require will be called on the main thread anyway, I will use index.cjs.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

awesome stuff, thanks!

writeFiles(DIR, {
'jest.config.js': `
module.exports = {prettierPath: require.resolve('prettier-2')};
module.exports = {prettierPath: require.resolve('prettier/index.cjs')};
Copy link
Member

Choose a reason for hiding this comment

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

why not just prettier?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is @fisker 's suggestion, perhaps to avoid unexpected use of Prettier 2?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I mean both index.cjs and index.mjs both should be tested, since user can pass prettier path.

Copy link
Member

Choose a reason for hiding this comment

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

We use require to load it, so ESM will fail

Copy link
Contributor

@fisker fisker Sep 25, 2023

Choose a reason for hiding this comment

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

The debug apis was actually missing in commonjs version at first, I added in prettier/prettier#13731

Copy link
Contributor

Choose a reason for hiding this comment

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

We use require to load it, so ESM will fail

Al right, didn't know.

@SimenB SimenB enabled auto-merge (squash) September 25, 2023 08:09
@SimenB SimenB merged commit 769ee6e into jestjs:main Sep 25, 2023
60 checks passed
@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 Oct 26, 2023
@SimenB
Copy link
Member

SimenB commented Oct 30, 2023

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Prettier v3 doesn't work with jest-snapshots
3 participants