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

Run prettier instance in worker_threads #3016

Merged
merged 37 commits into from
Jun 20, 2023

Conversation

sosukesuzuki
Copy link
Member

@sosukesuzuki sosukesuzuki commented Jun 6, 2023

This Pull Request runs instances of Prettier on worker_threads.

Prettier v3 depends on import() for loading configs and plugins. However, import() cannot be executed from the client of the VSCode extension, because the VSCode extension is run with vm.Script. For details, please see microsoft/vscode#130367.

We initially tried to solve this problem by introducing a language server, much like vscode-eslint. The language server can execute import(). Therefore, by running instances of Prettier on the language server, we should be able to resolve this issue.

I created a prototype of this. However, I determined that it is difficult to introduce a language server while maintaining the current behavior of prettier-vscode as much as possible.

Therefore, I decided to take another approach.

According to @liuxingbaoyu's comment (#2947 (comment)), VSCode extensions can use worker_threads and a Worker can execute import().

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

However, the current default instance of prettier-vscode is 2.x, which does not depend on import() and does not need to be run on Worker. Therefore, only those loaded from node_modules will be executed on Worker.

  • Run tests
  • Update the CHANGELOG.md with a summary of your changes

@sosukesuzuki

This comment was marked as resolved.

@sosukesuzuki

This comment was marked as resolved.

@fisker
Copy link
Member

fisker commented Jun 9, 2023

I've been very busy recently, and I'm not familiar with VSCode, but I'll try to find some time to take a look.

Comment on lines +77 to +89
let actual = doc.getText();

if (shouldRetry) {
for (let i = 0; i < 10; i++) {
if (text !== actual) {
break;
}
await wait(150);
await vscode.commands.executeCommand("editor.action.formatDocument");
actual = doc.getText();
}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Some code, like PHP files, can take a bit of time, like opening a text editor and then requiring plugins and huge parsers.
If you run formatDocument without waiting for that, it won't actually format the file.
We don't want that, so we'll only retry for some tests.
It's not the best way, but I don't think it's a problem anyway.

Copy link
Member

@fisker fisker left a comment

Choose a reason for hiding this comment

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

All comments are about styles, since you and liuxingbaoyu have it tested, I trust you.

@liuxingbaoyu
Copy link

Now the error message is not as pretty as the old one, maybe we can improve it!
image
vs
image

@fisker
Copy link
Member

fisker commented Jun 19, 2023

@sosukesuzuki
Copy link
Member Author

Pretty output with fisker's solution.

スクリーンショット 2023-06-20 2 18 33

@liuxingbaoyu
Copy link

To be honest I don't understand why it works. From what I understand it is using a different name.🤔
image

@sosukesuzuki
Copy link
Member Author

Just doing Object.assign was enough...?

@sosukesuzuki
Copy link
Member Author

No, just rejecting the error object passed from the worker produced the same output

@sosukesuzuki
Copy link
Member Author

Error serialization may not be necessary here. We can add it if we need it.

@JounQin
Copy link
Member

JounQin commented Dec 7, 2023

@sosukesuzuki

I don't quite understand, await import() should has been supported, but as we're using TypeScript, it may be compiled into require again.

But we can use the following hack:

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

@JounQin
Copy link
Member

JounQin commented Dec 7, 2023

Besides, the current solution does not consider yarn PnP support, synckit may also help.

sapegin added a commit to sapegin/emoji-console-log that referenced this pull request Apr 18, 2024
We can't use Prettier directly since extensions don't allow dynamic
imports: prettier/prettier-vscode#3016

Instead we find and read Prettier config file manually.

This solution is limited -- we only support JavaScript and JSON configs,
as well as configs in package.json.
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.

None yet

5 participants