Skip to content

Support Prettier v3#2947

Merged
ntotten merged 15 commits into
prettier:mainfrom
sosukesuzuki:support-v3
Apr 21, 2023
Merged

Support Prettier v3#2947
ntotten merged 15 commits into
prettier:mainfrom
sosukesuzuki:support-v3

Conversation

@sosukesuzuki

@sosukesuzuki sosukesuzuki commented Apr 6, 2023

Copy link
Copy Markdown
Contributor
  • Run tests
  • Update the CHANGELOG.md with a summary of your changes

Supports Prettier v3. Failed tests are not related to this PR. It also fails in main.

Comment thread src/PrettierEditService.ts Outdated
@sosukesuzuki

Copy link
Copy Markdown
Contributor Author

I found the problem of not being able to read ESM format configuration files...

@fisker

fisker commented Apr 11, 2023

Copy link
Copy Markdown
Member

I found the problem of not being able to read ESM format configuration files...

Prettier issue?

@sosukesuzuki

Copy link
Copy Markdown
Contributor Author

@fisker

Prettier issue?

Maybe No, and maybe it happens only with this test. When I actually ran it on VSCode, it worked fine.

@sosukesuzuki

Copy link
Copy Markdown
Contributor Author

I have found it difficult to test the loading of the configuration file with the way prettier-vscode testing now.
Even when testing the v3 folder, the configuration file is resolved using resolveConfigFile in Prettier 2.8.7. This is probably not a problem for users. It is a problem specific to testing this repository.

We will need to find a better way to test this in the future, for now I think we are good to go.

@sosukesuzuki
sosukesuzuki requested a review from fisker April 15, 2023 11:21
Comment thread src/PrettierEditService.ts Outdated
fisker
fisker previously approved these changes Apr 15, 2023

@fisker fisker left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Lgtm.

@fisker

fisker commented Apr 15, 2023

Copy link
Copy Markdown
Member

This one didn't awaited https://github.com/sosukesuzuki/prettier-vscode/blob/ab1b4ecfa540c634ddf2b2faaba1a60ab0a9ec57/src/ModuleResolver.ts#L327

I reviewed on cellphone, please double check Prettier api calls.

@fisker
fisker dismissed their stale review April 15, 2023 19:20

Need change

@fisker

fisker commented Apr 15, 2023

Copy link
Copy Markdown
Member

Comment thread src/PrettierEditService.ts Outdated
@sosukesuzuki
sosukesuzuki marked this pull request as ready for review April 16, 2023 08:08
@auto-assign
auto-assign Bot requested a review from ntotten April 16, 2023 08:08
Comment thread CHANGELOG.md

<!-- Check [Keep a Changelog](https://keepachangelog.com/) for recommendations on how to structure this file. -->

## [9.12.0]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be a major release. 10.0.0

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe we should release v10 after Prettier releases v3? This PR is not breaking anything, just add support for v3 alpha.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ntotten I agree with fisker. If you too can agree with this, please merge and release.

@fisker fisker Apr 20, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems there is a release workflow. So I guess we can create a v9.12.0 tag to trigger release?

release:
runs-on: ubuntu-latest
needs: package
if: startsWith(github.ref, 'refs/tags/v')

But let's wait for ntotten to respond first.

@ntotten

ntotten commented Apr 17, 2023

Copy link
Copy Markdown
Contributor

Let me know if you need help on the tests.

@fisker

fisker commented Apr 23, 2023

Copy link
Copy Markdown
Member

@sosukesuzuki You are right, even after I fix the import() error, it still can't load config and plugins.

@sosukesuzuki

sosukesuzuki commented Apr 23, 2023

Copy link
Copy Markdown
Contributor Author

Oh... So we must find the way execute import expr in prettier vscode...

@fisker

fisker commented Apr 23, 2023

Copy link
Copy Markdown
Member

Maybe this will work? LinqLover/downstream-repository-mining@9f57b01

@fisker

fisker commented Apr 23, 2023

Copy link
Copy Markdown
Member

Change to

var prettierPromise = new Function("x", "return import(x)")("./index.mjs");
["ERROR" - 19:48:21] A dynamic import callback was not specified.
TypeError: A dynamic import callback was not specified.

@kachkaev

This comment was marked as off-topic.

@fisker

This comment was marked as off-topic.

@sosukesuzuki

sosukesuzuki commented Apr 23, 2023

Copy link
Copy Markdown
Contributor Author

@sosukesuzuki

Copy link
Copy Markdown
Contributor Author

This is my guess: the reason why ESLint's Flat Config works fine may be because it is the Language Server that is doing the dynamic import. The client may not be able to do dynamic imports.

@sosukesuzuki

Copy link
Copy Markdown
Contributor Author

We may need to adopt that same architecture... Separating the server that runs Prettier and client reflects values to the editor.

@fisker

fisker commented Apr 23, 2023

Copy link
Copy Markdown
Member

I also found an issue seems related to esm linked to vscode iteration plan, but looks like not useful.

opral/inlang#452

@kachkaev

kachkaev commented Apr 23, 2023

Copy link
Copy Markdown
Member

I tried VS Code Insiders, but it did not help:

["INFO" - 16:38:35] Extension Name: esbenp.prettier-vscode.
["INFO" - 16:38:35] Extension Version: 9.12.0.
["ERROR" - 16:38:37] Error handling text editor change
["ERROR" - 16:38:37] A dynamic import callback was not specified.
TypeError: A dynamic import callback was not specified.
	at new NodeError (node:internal/errors:387:5)
	at importModuleDynamicallyCallback (node:internal/process/esm_loader:39:9)
	at Object.<anonymous> (/path/to/project/node_modules/.pnpm/prettier@3.0.0-alpha.10/node_modules/prettier/index.cjs:647:23)
	at u._compile (/Applications/Visual Studio Code - Insiders.app/Contents/Resources/app/out/vs/loader.js:4:1271)
	at Module._extensions..js (node:internal/modules/cjs/loader:1243:10)
	at Module.load (node:internal/modules/cjs/loader:1058:32)
	at Module._load (node:internal/modules/cjs/loader:893:12)
	at f._load (node:electron/js2c/asar_bundle:2:13330)
	at b._load (/Applications/Visual Studio Code - Insiders.app/Contents/Resources/app/out/vs/workbench/api/node/extensionHostProcess.js:124:29918)
VS Code Insiders
Version: 1.78.0-insider
Commit: b19017cea80b6157aa8214c984a70022e77526f2
Date: 2023-04-21T05:39:53.263Z (2 days ago)

@sosukesuzuki

Copy link
Copy Markdown
Contributor Author

I think we need to change prettier-vscode to a client/server architecture like a eslint-vscode for releasing Prettier v3.

@ntotten @fisker What do you think? Do you have an alternative plan?

@fisker

fisker commented Apr 26, 2023

Copy link
Copy Markdown
Member

I don't have other plan.

@sosukesuzuki

Copy link
Copy Markdown
Contributor Author

Starting April 29th, my university and work will have 9 days off, so I'll try to implement it there.

@sosukesuzuki

Copy link
Copy Markdown
Contributor Author

I'm working on https://github.com/sosukesuzuki/prettier-language-server. It will still take time. This is a more difficult task than I thought.

@liuxingbaoyu

Copy link
Copy Markdown

Is it possible for us to start a subprocess to handle it?
This will be a bit slower, but it shouldn't affect much.

@sosukesuzuki

Copy link
Copy Markdown
Contributor Author

Sorry, what do you mean?

@liuxingbaoyu

Copy link
Copy Markdown

microsoft/vscode#130367 (comment)
Like this one.
We start a new node process and run prettier inside it.

@sosukesuzuki

Copy link
Copy Markdown
Contributor Author

Oh.. I'll try it later. thank you

@liuxingbaoyu

Copy link
Copy Markdown

image
A new discovery!
worker_threads in vscode is accessible. We can try to take advantage of it!

@fisker

fisker commented Jun 3, 2023

Copy link
Copy Markdown
Member

worker_threads doesn't support chdir, should be careful.

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