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

fix: use synckit to support Prettier 3 #199

Merged
merged 25 commits into from
Feb 6, 2024
Merged

fix: use synckit to support Prettier 3 #199

merged 25 commits into from
Feb 6, 2024

Conversation

idahogurl
Copy link
Owner

@idahogurl idahogurl commented Dec 1, 2023

What

Closes #171

Breaking Changes

  • prettier-eslint must be installed locally by the user
  • Prettier v3.0 is now required
  • ESLint >= v8.52.0 is now required

Why

Prettier 3 added a dynamic import() in its .cjs file, making ESM support required. VS Code only supports CommonJS (CJS) however. According to this comment prettier/prettier-vscode#2947 (comment), VSCode extensions can use worker_threads and a Worker can execute import().

Thus, this PR makes it so that instances of prettier-eslint are run on Worker of worker_threads.

@JounQin
Copy link

JounQin commented Dec 7, 2023

@idahogurl AFAIK, you can use await import() directly in cjs, can't you? For legacy TypeScript, you can use this hack:

https://github.com/just-jeb/angular-builders/blob/e8d063cd16346514d55ec6e25d41c184a999ca42/packages/custom-webpack/src/utils.ts#L68-L70

@idahogurl
Copy link
Owner Author

idahogurl commented Dec 8, 2023

@idahogurl AFAIK, you can use await import() directly in cjs, can't you? For legacy TypeScript, you can use this hack:

https://github.com/just-jeb/angular-builders/blob/e8d063cd16346514d55ec6e25d41c184a999ca42/packages/custom-webpack/src/utils.ts#L68-L70

@JounQin That would work if I was bundling prettier since esbuild would convert the ESM to CJS but it is an external dependency.

@JounQin
Copy link

JounQin commented Dec 8, 2023

That doesn't matter with external and no need to bundle at all for regular commonjs, but I'm not so familiar with VSCode/vmscripts.

Regularly, await import('esm.mjs') is just supported, so await import('prettier') should just work.

If it doesn't work, then synckit should be a good choice.

src/extension.js Outdated Show resolved Hide resolved
src/worker.js Outdated Show resolved Hide resolved
src/worker.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@JounQin
Copy link

JounQin commented Dec 9, 2023

If you need any help, I can help to raise a new PR based on this one later for you.

src/extension.js Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/worker.js Outdated Show resolved Hide resolved
Co-authored-by: JounQin <[email protected]>
src/extension.js Outdated Show resolved Hide resolved
src/extension.js Outdated Show resolved Hide resolved
src/extension.js Outdated Show resolved Hide resolved
@JounQin
Copy link

JounQin commented Dec 9, 2023

Can you update the PR title accordingly?

@idahogurl idahogurl changed the title fix: use worker_threads to support Prettier 3 fix: use synckit to support Prettier 3 Dec 9, 2023
package.json Outdated Show resolved Hide resolved
@idahogurl idahogurl marked this pull request as ready for review December 9, 2023 05:55
@netux
Copy link

netux commented Dec 10, 2023

I did some quick debugging and found out there is still a bottleneck with the worker having to load prettier-eslint, which on my machine takes roughly 2-3 seconds.

Also the preload did nothing for me because, from a VSCode window reload with a document already opened, the extension is only activated when the formatter is run.

I think a solution would be to:

  1. Add onStartupFinished to the "activationEvents" field in the package.json, to make sure the extension is ready after VSCode starts.
  2. On load, run formatText() with some dummy document to force the engine to go through the code paths that a real document would go through, and load all the necessary things that way.

@JounQin
Copy link

JounQin commented Dec 10, 2023

@netux I think you can just raise a PR for your proposal.

@netux
Copy link

netux commented Dec 10, 2023

@netux I think you can just raise a PR for your proposal.

Good idea! I have opened #202 🙂.

* fix: use synckit with ESM correctly

* chore: add .vsix files into .gitignore
package.json Show resolved Hide resolved
function waitForActiveSupportedDocument() {
if (
!window.activeTextEditor
|| !supportedLanguages.includes(window.activeTextEditor.document.languageId)
Copy link

Choose a reason for hiding this comment

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

I believe supportedLanguages should not be hard coded because of 3rd-party plugins. But it can be done in next minor version, I'd like to PR after v3 released later.

@JounQin
Copy link

JounQin commented Dec 15, 2023

I think we are just ready to go! Congratulations!

@JounQin JounQin mentioned this pull request Jan 15, 2024
@novarolV
Copy link

I have opened #206 to address an issue on Windows.

* fix: resolve path to file url correctly

* check if path absolute
package.json Outdated Show resolved Hide resolved
@idahogurl idahogurl merged commit 41fe336 into master Feb 6, 2024
1 check passed
@idahogurl idahogurl deleted the 171-prettier-3-0 branch February 6, 2024 18:30
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.

Prettier 3.0
4 participants