search

prettier / prettier-vscode

Visual Studio Code extension for Prettier
https://marketplace.visualstudio.com/items?itemName=esbenp.prettier-vscode
MIT License
5.04k stars 446 forks source link

Support Prettier v3 #2947

Closed sosukesuzuki closed 1 year ago

sosukesuzuki commented 1 year ago

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

sosukesuzuki commented 1 year ago

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

fisker commented 1 year ago

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

Prettier issue?

sosukesuzuki commented 1 year ago

@fisker

Prettier issue?

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

sosukesuzuki commented 1 year ago

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.

fisker commented 1 year ago

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 commented 1 year ago

And this https://github.com/sosukesuzuki/prettier-vscode/blob/ab1b4ecfa540c634ddf2b2faaba1a60ab0a9ec57/src/TemplateService.ts#L23

ntotten commented 1 year ago

Let me know if you need help on the tests.

fisker commented 1 year ago

Close and reopen to apply #2963

fisker commented 1 year ago

🚀 All green!

kachkaev commented 1 year ago

Just tried this PR locally – it worked for me in a project that uses Prettier 3.0.0-alpha.6 🎉 Thanks for this PR @sosukesuzuki, I look forward to using a new release!

sosukesuzuki commented 1 year ago

Can we merge and release?

kachkaev commented 1 year ago

@ntotten could you please help us with testing Prettier 3 alpha by cutting a new release? It’d be nice to do this before https://github.com/prettier/prettier/issues/13142 – we are nearly there!

If you are too busy (which is totally fine), who else could do the release?

ntotten commented 1 year ago

I'll try to take a look first thing in the morning.

kachkaev commented 1 year ago

Thanks for cutting the release @ntotten!

Maybe it’s something with my local setup, but I seem to be unable to use Prettier 3 in the new extension. Here is the error:

"INFO" - 16:26:32] Extension Name: esbenp.prettier-vscode.
["INFO" - 16:26:32] Extension Version: 9.12.0.
["ERROR" - 16:26:32] Error handling text editor change
["ERROR" - 16:26:32] Invalid host defined options
TypeError: Invalid host defined options
    at Object.<anonymous> (/path/to/my/project/node_modules/prettier/index.cjs:647:23)
    at Module.u._compile (/Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/loader.js:4:1271)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1220:10)
    at Module.load (node:internal/modules/cjs/loader:1035:32)
    at Module._load (node:internal/modules/cjs/loader:876:12)
    at Function.c._load (node:electron/js2c/asar_bundle:5:13343)
    at Function.m._load (/Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/workbench/api/node/extensionHostProcess.js:124:14199)
    at Function.h._load (/Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/workbench/api/node/extensionHostProcess.js:119:11871)
    at Function.I._load (/Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/workbench/api/node/extensionHostProcess.js:119:11264)
    at Module.require (node:internal/modules/cjs/loader:1059:19)
    at Module.require (/Users/ak/.vscode/extensions/github.copilot-1.83.41/dist/extension.js:8:366165)
    at t.ModuleResolver.g (/Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/loader.js:4:647)
    at t.ModuleResolver.loadNodeModule (/Users/ak/.vscode/extensions/esbenp.prettier-vscode-9.12.0/dist/extension.js:1:6406)
    at t.ModuleResolver.getPrettierInstance (/Users/ak/.vscode/extensions/esbenp.prettier-vscode-9.12.0/dist/extension.js:1:4086)
    at t.default.handleActiveTextEditorChanged (/Users/ak/.vscode/extensions/esbenp.prettier-vscode-9.12.0/dist/extension.js:1:9843)
    at t.default.handleActiveTextEditorChangedSync (/Users/ak/.vscode/extensions/esbenp.prettier-vscode-9.12.0/dist/extension.js:1:9445)
    at t.default.registerDisposables (/Users/ak/.vscode/extensions/esbenp.prettier-vscode-9.12.0/dist/extension.js:1:12288)
    at /Users/ak/.vscode/extensions/esbenp.prettier-vscode-9.12.0/dist/extension.js:1:77862

Error stack points to /path/to/my/project/node_modules/prettier/index.cjs:647:23:

// ...

// src/main/version.evaluate.cjs
var require_version_evaluate = __commonJS({
  "src/main/version.evaluate.cjs"(exports2, module2) {
    module2.exports = "3.0.0-alpha.6";
  }
});

// src/index.cjs
var prettierPromise = import("./index.mjs"); //                 👈 👀 this line
var functionNames = [
  "formatWithCursor",
  "format",
  "check",
  "resolveConfig",
  "resolveConfigFile",
  "clearConfigCache",
  "getFileInfo",
  "getSupportInfo"
];

// ...

Can someone try reproducing the problem? You can use this repo: https://github.com/gicentre/prettier-plugin-elm

cd /tmp
git clone https://github.com/gicentre/prettier-plugin-elm.git
cd prettier-plugin-elm

yarn install
yarn bin prettier
## 👀 /tmp/prettier-plugin-elm/node_modules/prettier/bin/prettier.cjs
yarn prettier --version
## 👀 3.0.0-alpha.6

code .
## 💻 open Prettier Output tab in VS Code
## 💻 edit README.md
## 💻 press save

Note that yarn why prettier will show two more local Prettier versions, which I use in tests. They should not affect the extension and I can also see the same output in another project (which is not open source).

fisker commented 1 year ago

I don't have a laptop now, what will happen if we config prettierPath in settings to ./node_modules/prettier/index.mjs? (My guess is the error will be "can not require es module")

fisker commented 1 year ago

VSCode extension doesnt support dynamic import? https://github.com/microsoft/vscode/issues/130367

kachkaev commented 1 year ago

Interestingly, when I checked out and ran the extension from this branch, it did work. Banned import("./index.mjs") looks a bit crazy, but seeing it banned only in prod is even crazier. We should probably release Prettier 3.0.0 only after the bug with the VS Code extension is investigated and fixed. Not sure I have any good ideas right now to be honest.

Does anyone know someone from the VS Code team to ask them for advice? Prettier 3 uses a somewhat bespoke combo of CJS+ESM, but it makes total sense assuming that import("./index.mjs") just works™

sosukesuzuki commented 1 year ago

I found a tweet maybe rerated?: https://twitter.com/slicknet/status/1559619176230047744

If you’re using Node.js and get a “invalid host defined options” error, it means you’re trying to use v8-compile-cache with ESM modules. That doesn’t work. Sadly the only solution is to remove v8-compile-cache.

sosukesuzuki commented 1 year ago

I don't have a laptop now, what will happen if we config prettierPath in settings to ./node_modules/prettier/index.mjs? (My guess is the error will be "can not require es module")

Errors on configuration prettierPath: ./node_modules/prettier/index.mjs

["INFO" - 14:58:56] Extension Name: esbenp.prettier-vscode.
["INFO" - 14:58:56] Extension Version: 9.12.0.
["ERROR" - 14:58:56] Error loading node module '/Users/sosuke.suzuki/development/prettier-v3-vscode-try/node_modules/prettier/index.mjs'
["ERROR" - 14:58:56] require() of ES Module /Users/sosuke.suzuki/development/prettier-v3-vscode-try/node_modules/prettier/index.mjs not supported.
Instead change the require of /Users/sosuke.suzuki/development/prettier-v3-vscode-try/node_modules/prettier/index.mjs to a dynamic import() which is available in all CommonJS modules.
Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/sosuke.suzuki/development/prettier-v3-vscode-try/node_modules/prettier/index.mjs not supported.
Instead change the require of /Users/sosuke.suzuki/development/prettier-v3-vscode-try/node_modules/prettier/index.mjs to a dynamic import() which is available in all CommonJS modules.
    at c._load (node:electron/js2c/asar_bundle:5:13343)
    at m._load (/Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/workbench/api/node/extensionHostProcess.js:124:14199)
    at h._load (/Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/workbench/api/node/extensionHostProcess.js:119:11871)
    at I._load (/Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/workbench/api/node/extensionHostProcess.js:119:11264)
    at t.ModuleResolver.g (/Applications/Visual Studio Code.app/Contents/Resources/app/out/vs/loader.js:4:647)
    at t.ModuleResolver.loadNodeModule (/Users/sosuke.suzuki/.vscode/extensions/esbenp.prettier-vscode-9.12.0/dist/extension.js:1:6406)
    at t.ModuleResolver.getPrettierInstance (/Users/sosuke.suzuki/.vscode/extensions/esbenp.prettier-vscode-9.12.0/dist/extension.js:1:4086)
    at t.default.handleActiveTextEditorChanged (/Users/sosuke.suzuki/.vscode/extensions/esbenp.prettier-vscode-9.12.0/dist/extension.js:1:9843)
    at t.default.handleActiveTextEditorChangedSync (/Users/sosuke.suzuki/.vscode/extensions/esbenp.prettier-vscode-9.12.0/dist/extension.js:1:9445)
    at t.default.registerDisposables (/Users/sosuke.suzuki/.vscode/extensions/esbenp.prettier-vscode-9.12.0/dist/extension.js:1:12288)
    at /Users/sosuke.suzuki/.vscode/extensions/esbenp.prettier-vscode-9.12.0/dist/extension.js:1:77862
sosukesuzuki commented 1 year ago

In production env, our extensions is crashed by ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING

sosukesuzuki commented 1 year ago

Perhaps it is not possible to call dynamic import from VSCode extensions at this time. What should we do...?

fisker commented 1 year ago

I remember ESLint's new flat config is loaded via import(), does it work?

sosukesuzuki commented 1 year ago

I remember ESLint's new flat config is loaded via import(), does it work?

I don't know, I'll look into it.

note: https://github.com/eslint/eslint/blob/main/lib/eslint/flat-eslint.js#L316

sosukesuzuki commented 1 year ago

ESLint Flat Config works well with VSCode extension (config file is ESM).

sosukesuzuki commented 1 year ago

Why prettier-vscode is executed in vm.Script env...? prettier-vscode works well when run from the Run Extension of VSCode's debugging screen.

fisker commented 1 year ago

I'm going to bundle a real CommonJS version https://github.com/prettier/prettier/pull/14746

fisker commented 1 year ago

I'll publish a new version once the test passes.

sosukesuzuki commented 1 year ago

@fisker With this way, we can load the Prettier itself, but not the plug-ins or configuration files?

fisker commented 1 year ago

I don't know yet, testing...

fisker commented 1 year ago

Published prettier@prettier@3.0.0-alpha.7-for-vscode from my pc.

fisker commented 1 year ago

Forgot add ~--latest~--next tag...

fisker commented 1 year ago

And I can't unpublish it... shit

fisker commented 1 year ago

Published prettier@3.0.0-alpha.9-for-vscode

sosukesuzuki commented 1 year ago

My local VSCode with 3.0.0-alpha.9-for-vscode output:

["INFO" - 17:55:38] Extension Name: esbenp.prettier-vscode.
["INFO" - 17:55:38] Extension Version: 9.12.0.
["ERROR" - 17:55:38] Error handling text editor change
["ERROR" - 17:55:38] import_mockable2.default.findParentDir is not a function
TypeError: import_mockable2.default.findParentDir is not a function
    at /Users/sosuke.suzuki/development/prettier-v3-vscode-try/node_modules/prettier/index.cjs:20389:43
    at /Users/sosuke.suzuki/development/prettier-v3-vscode-try/node_modules/prettier/index.cjs:18723:23
    at mem.cacheKey (/Users/sosuke.suzuki/development/prettier-v3-vscode-try/node_modules/prettier/index.cjs:20418:36)
    at /Users/sosuke.suzuki/development/prettier-v3-vscode-try/node_modules/prettier/index.cjs:18723:23
    at Object.<anonymous> (/Users/sosuke.suzuki/development/prettier-v3-vscode-try/node_modules/prettier/index.cjs:20730:51)
    at t.default.getSelectors (/Users/sosuke.suzuki/.vscode/extensions/esbenp.prettier-vscode-9.12.0/dist/extension.js:1:10587)
    at t.default.handleActiveTextEditorChanged (/Users/sosuke.suzuki/.vscode/extensions/esbenp.prettier-vscode-9.12.0/dist/extension.js:1:10007)
fisker commented 1 year ago

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

sosukesuzuki commented 1 year ago

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

fisker commented 1 year ago

Maybe this will work? https://github.com/LinqLover/downstream-repository-mining/commit/9f57b01d860961103460f27de416cfe5b95854e6

fisker commented 1 year ago

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 commented 1 year ago

Forgot add --latest--next tag...

It is possible to set tags (latest, canary etc.) after the publish: https://docs.npmjs.com/adding-dist-tags-to-packages

So in the future if we accidentally publish latest by accident, we can run

npm dist-tag add prettier@2.8.7 latest

and this will ‘undo’ 3.0.0-alpha.N being latest

(we can collapse this comment as this is offtopic)

fisker commented 1 year ago

Didn't know that, thanks.

sosukesuzuki commented 1 year ago

Our problem is maybe caused by vscode-loader (ref: https://github.com/microsoft/vscode/issues/130367#issuecomment-1030687214).

It uses vm.Script (ref: https://github.com/microsoft/vscode-loader/blob/15c69659545fa41e3290a7ad060f2ceaaaef5fa1/src/loader.js#L859)

Or vscode/src/vs/loader.js ? https://github.com/microsoft/vscode/blob/main/src/vs/loader.js#L797

sosukesuzuki commented 1 year ago

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 commented 1 year ago

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

fisker commented 1 year ago

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

https://github.com/inlang/inlang/issues/452

kachkaev commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

I don't have other plan.

sosukesuzuki commented 1 year ago

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

sosukesuzuki commented 1 year ago

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 commented 11 months ago

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