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
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
fe36bd8
Implement worker
sosukesuzuki Jun 3, 2023
89adebf
Fix workers
sosukesuzuki Jun 4, 2023
50e9c19
Fix...
sosukesuzuki Jun 4, 2023
ff49d1f
Promise
sosukesuzuki Jun 4, 2023
ff3da84
Fix
sosukesuzuki Jun 4, 2023
1692842
embed logger
sosukesuzuki Jun 4, 2023
242489b
More log...
sosukesuzuki Jun 4, 2023
b4f151e
It works...
sosukesuzuki Jun 4, 2023
1681bc3
Fix
sosukesuzuki Jun 4, 2023
87faf54
Avoid clone error
sosukesuzuki Jun 4, 2023
0b7305e
More detailed logs
sosukesuzuki Jun 4, 2023
a6d6a8e
Failed only one test
sosukesuzuki Jun 4, 2023
419a3e4
Refactor
sosukesuzuki Jun 4, 2023
6052ff6
Fix
sosukesuzuki Jun 4, 2023
11db0da
Fix
sosukesuzuki Jun 7, 2023
b8ac001
Fix
sosukesuzuki Jun 7, 2023
c9c7477
Skip plugins tests
sosukesuzuki Jun 8, 2023
7b95303
Skip pnpm plugin test
sosukesuzuki Jun 8, 2023
5b7bbcb
Skip plugins tests for only linux
sosukesuzuki Jun 8, 2023
a8e5a6a
Revert skipping
sosukesuzuki Jun 8, 2023
9414fd9
Await for php testing
sosukesuzuki Jun 10, 2023
39eaff0
Fix retry
sosukesuzuki Jun 10, 2023
9b8ca48
Clean up
sosukesuzuki Jun 10, 2023
93d8f71
Remove log file
sosukesuzuki Jun 10, 2023
9b33cd4
Add log and refactoring
sosukesuzuki Jun 10, 2023
f8f51be
Remove comment
sosukesuzuki Jun 10, 2023
7ff3997
Fix script for `vscode:prepublish`
sosukesuzuki Jun 10, 2023
bd30842
Use copy-webpack-plugin
sosukesuzuki Jun 10, 2023
ac6e320
Fix typo
sosukesuzuki Jun 14, 2023
a80d6bc
Add `await`
sosukesuzuki Jun 14, 2023
e858752
Refactor with Map
sosukesuzuki Jun 14, 2023
f1cda05
Requite instance while calling method if instance does not exist
sosukesuzuki Jun 14, 2023
4c1fdc8
Update changelog
sosukesuzuki Jun 16, 2023
901d664
Introduce simple serializer
sosukesuzuki Jun 16, 2023
6b43bfe
Merge branch 'main' into in-worker
sosukesuzuki Jun 19, 2023
eaf9e2f
Pretty output
sosukesuzuki Jun 19, 2023
941d91b
Remove error serialization
sosukesuzuki Jun 20, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .eslintignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
/src/worker/*.js
Empty file added log.txt
Empty file.
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,13 @@
"lint": "eslint -c .eslintrc.js --ext .ts .",
"pretest": "yarn test-compile && cd test-fixtures/plugins && yarn install && cd ../plugins-pnpm && pnpm i && cd ../outdated && yarn install && cd ../module && yarn install && cd ../specific-version && yarn install && cd ../explicit-dep && yarn install && cd implicit-dep && yarn install && cd ../../v3 && yarn install",
"prettier": "prettier --write '**/*.{ts,json,md,hbs,yml,js}'",
"test-compile": "yarn clean && tsc -p ./ && yarn webpack",
"test-compile": "yarn clean && tsc -p ./ && yarn webpack && cp -r ./src/worker ./out",
"test": "node ./out/test/runTests.js",
"version": "node ./scripts/version.js && git add CHANGELOG.md",
"vscode:prepublish": "webpack --mode production",
"watch": "tsc --watch -p ./",
"webpack-dev": "webpack --mode development --watch",
"webpack": "webpack --mode development",
"webpack": "webpack --mode development && cp -r ./src/worker ./dist",
"postinstall": "husky install",
"chrome": "yarn webpack && vscode-test-web --browserType=chromium --extensionDevelopmentPath=. ."
},
Expand Down
45 changes: 14 additions & 31 deletions src/ModuleResolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
PrettierVSCodeConfig,
} from "./types";
import { getConfig, getWorkspaceRelativePath } from "./util";
import { PrettierWorkerInstance } from "./PrettierWorkerInstance";

const minPrettierVersion = "1.13.0";
declare const __webpack_require__: typeof require;
Expand Down Expand Up @@ -69,7 +70,8 @@ function globalPathGet(packageManager: PackageManagers): string | undefined {
export class ModuleResolver implements ModuleResolverInterface {
private findPkgCache: Map<string, string>;
private ignorePathCache = new Map<string, string>();
private path2Module = new Map<string, PrettierNodeModule>();

private path2Module = new Map<string, PrettierWorkerInstance>();

constructor(private loggingService: LoggingService) {
this.findPkgCache = new Map();
Expand All @@ -85,7 +87,7 @@ export class ModuleResolver implements ModuleResolverInterface {
*/
public async getPrettierInstance(
fileName: string
): Promise<PrettierNodeModule | undefined> {
): Promise<PrettierNodeModule | PrettierWorkerInstance | undefined> {
if (!workspace.isTrusted) {
this.loggingService.logDebug(UNTRUSTED_WORKSPACE_USING_BUNDLED_PRETTIER);
return prettier;
Expand Down Expand Up @@ -148,7 +150,8 @@ export class ModuleResolver implements ModuleResolverInterface {
}
}

let moduleInstance: PrettierNodeModule | undefined = undefined;
let moduleInstance: PrettierWorkerInstance | undefined = undefined;

if (modulePath !== undefined) {
this.loggingService.logDebug(
`Local prettier module path: '${modulePath}'`
Expand All @@ -159,7 +162,7 @@ export class ModuleResolver implements ModuleResolverInterface {
return moduleInstance;
} else {
try {
moduleInstance = this.loadNodeModule<PrettierNodeModule>(modulePath);
moduleInstance = new PrettierWorkerInstance(modulePath);
if (moduleInstance) {
this.path2Module.set(modulePath, moduleInstance);
}
Expand All @@ -178,31 +181,23 @@ export class ModuleResolver implements ModuleResolverInterface {
}

if (moduleInstance) {
// If the instance is missing `format`, it's probably
// not an instance of Prettier
const isPrettierInstance = !!moduleInstance.format;
const isValidVersion =
moduleInstance.version &&
!!moduleInstance.getSupportInfo &&
!!moduleInstance.getFileInfo &&
!!moduleInstance.resolveConfig &&
semver.gte(moduleInstance.version, minPrettierVersion);

if (!isPrettierInstance && prettierPath) {
const version = await moduleInstance.import();
Copy link
Member

Choose a reason for hiding this comment

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

We can't really access all properties of the module. I'd do

await moduleInstance.get('version')

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't really need to access any properties other than version, so I think we can leave it as is.


if (!version && prettierPath) {
this.loggingService.logError(INVALID_PRETTIER_PATH_MESSAGE);
return undefined;
}

const isValidVersion = version && semver.gte(version, minPrettierVersion);

if (!isValidVersion) {
this.loggingService.logInfo(
`Attempted to load Prettier module from ${modulePath}`
);
this.loggingService.logError(OUTDATED_PRETTIER_VERSION_MESSAGE);
return undefined;
} else {
this.loggingService.logDebug(
`Using prettier version ${moduleInstance.version}`
);
this.loggingService.logDebug(`Using prettier version ${version}`);
}
return moduleInstance;
} else {
Expand Down Expand Up @@ -327,6 +322,7 @@ export class ModuleResolver implements ModuleResolverInterface {
await prettier.clearConfigCache();
this.path2Module.forEach((module) => {
try {
// eslint-disable-next-line @typescript-eslint/no-floating-promises
module.clearConfigCache();
} catch (error) {
this.loggingService.logError("Error clearing module cache.", error);
Expand All @@ -341,19 +337,6 @@ export class ModuleResolver implements ModuleResolverInterface {
: require;
}

// Source: https://github.com/microsoft/vscode-eslint/blob/master/server/src/eslintServer.ts
private loadNodeModule<T>(moduleName: string): T | undefined {
try {
return this.nodeModuleLoader(moduleName);
} catch (error) {
this.loggingService.logError(
`Error loading node module '${moduleName}'`,
error
);
}
return undefined;
}

private resolveNodeModule(moduleName: string, options?: { paths: string[] }) {
try {
return this.nodeModuleLoader.resolve(moduleName, options);
Expand Down
3 changes: 2 additions & 1 deletion src/PrettierEditService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
RangeFormattingOptions,
} from "./types";
import { getConfig } from "./util";
import { PrettierWorkerInstance } from "./PrettierWorkerInstance";

interface ISelectors {
rangeLanguageSelector: ReadonlyArray<DocumentFilter>;
Expand Down Expand Up @@ -252,7 +253,7 @@ export default class PrettierEditService implements Disposable {
* Build formatter selectors
*/
private getSelectors = async (
prettierInstance: PrettierModule,
prettierInstance: PrettierModule | PrettierWorkerInstance,
uri?: Uri
): Promise<ISelectors> => {
const { languages } = await prettierInstance.getSupportInfo();
Expand Down
114 changes: 114 additions & 0 deletions src/PrettierWorkerInstance.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
import { Worker } from "worker_threads";
import * as url from "url";
import * as path from "path";
import {
PrettierFileInfoOptions,
PrettierFileInfoResult,
PrettierOptions,
PrettierSupportLanguage,
} from "./types";

const worker = new Worker(
url.pathToFileURL(path.join(__dirname, "/worker/prettier-instance-worker.js"))
);

export class PrettierWorkerInstance {
private importResolver: {
resolve: (version: string) => void;
reject: (version: string) => void;
} | null = null;

private callMethodResolvers: {
id: number;
resolve: (value: unknown) => void;
reject: (value: unknown) => void;
}[] = [];
Copy link
Member

@fisker fisker Jun 13, 2023

Choose a reason for hiding this comment

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

Maybe we can use Map or Object to store? so we can get it easier.

callMethodResolvers.findIndex
callMethodResolvers.splice

to

callMethodResolvers.get(id)
callMethodResolvers.delete(id)


private currentCallMethodId = 0;

public version: string | null = null;

constructor(private modulePath: string) {
worker.on("message", ({ type, payload }) => {
switch (type) {
case "import": {
this.importResolver?.resolve(payload.version);
this.version = payload.version;
break;
}
case "callMethod": {
const resolver = this.callMethodResolvers.find(({ id }) => {
return id === payload.id;
});
if (resolver) {
if (payload.isError) {
resolver.reject(payload.result);
} else {
resolver.resolve(payload.result);
}
}
break;
}
}
});
}

public async import(): Promise</* version of imported prettier */ string> {
const promise = new Promise<string>((resolve, reject) => {
this.importResolver = { resolve, reject };
});
worker.postMessage({
type: "import",
payload: { modulePath: this.modulePath },
});
return promise;
}

public async format(
source: string,
options?: PrettierOptions
): Promise<string> {
const result = this.callMethod("format", [source, options]);
return result;
}

public async getSupportInfo(): Promise<{
languages: PrettierSupportLanguage[];
}> {
const result = await this.callMethod("getSupportInfo", []);
Copy link
Member

@fisker fisker Jun 13, 2023

Choose a reason for hiding this comment

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

Why these two methods, one awaited, but another one not.

return result;
}

public async clearConfigCache(): Promise<void> {
await this.callMethod("clearConfigCache", []);
}

public async getFileInfo(
filePath: string,
fileInfoOptions?: PrettierFileInfoOptions
): Promise<PrettierFileInfoResult> {
const result = await this.callMethod("getFileInfo", [
filePath,
fileInfoOptions,
]);
return result;
}

private callMethod(methodName: string, methodArgs: unknown[]): Promise<any> {
const callMethodId = this.currentCallMethodId++;
// log(JSON.stringify({ methodName, methodArgs, callMethodId }));
const promise = new Promise((resolve, reject) => {
this.callMethodResolvers.push({ id: callMethodId, resolve, reject });
});
worker.postMessage({
type: "callMethod",
payload: {
id: callMethodId,
modulePath: this.modulePath,
methodName,
methodArgs,
},
});
return promise;
}
}
21 changes: 19 additions & 2 deletions src/test/suite/format.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,11 @@ export function putBackPrettierRC(done: Done) {
* @param base base URI
* @returns source code and resulting code
*/
export async function format(workspaceFolderName: string, testFile: string) {
export async function format(
workspaceFolderName: string,
testFile: string,
shouldRetry = false
) {
const base = getWorkspaceFolderUri(workspaceFolderName);
const absPath = path.join(base.fsPath, testFile);
const doc = await vscode.workspace.openTextDocument(absPath);
Expand All @@ -70,10 +74,23 @@ export async function format(workspaceFolderName: string, testFile: string) {
console.time(testFile);
await vscode.commands.executeCommand("editor.action.formatDocument");

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();
}
}

Comment on lines +77 to +89
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.

// eslint-disable-next-line no-console
console.timeEnd(testFile);

return { actual: doc.getText(), source: text };
return { actual, source: text };
}
/**
* Compare prettier's output (default settings)
Expand Down
6 changes: 5 additions & 1 deletion src/test/suite/module.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,11 @@ suite("Test module resolution", function () {
});

test("it loads plugin referenced in dependency module", async () => {
const { actual } = await format("module-plugin", "index.js");
const { actual } = await format(
"module-plugin",
"index.js",
/* shouldRetry */ true
);
const expected = await getText("module-plugin", "index.result.js");
assert.equal(actual, expected);
});
Expand Down
6 changes: 5 additions & 1 deletion src/test/suite/plugins.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@ import { format, getText } from "./format.test";
suite("Test plugins", function () {
this.timeout(10000);
test("it formats with plugins", async () => {
const { actual } = await format("plugins", "index.php");
const { actual } = await format(
"plugins",
"index.php",
/* shouldRetry */ true
);
const expected = await getText("plugins", "index.result.php");
assert.equal(actual, expected);
});
Expand Down
5 changes: 4 additions & 1 deletion src/types.d.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import * as prettier from "prettier";
import { TextDocument } from "vscode";
import { PrettierWorkerInstance } from "./PrettierWorkerInstance";

type PrettierSupportLanguage = {
vscodeLanguageIds?: string[];
Expand All @@ -25,7 +26,9 @@ type PrettierModule = {
};

type ModuleResolverInterface = {
getPrettierInstance(fileName: string): Promise<PrettierModule | undefined>;
getPrettierInstance(
fileName: string
): Promise<PrettierModule | PrettierWorkerInstance | undefined>;
getResolvedIgnorePath(
fileName: string,
ignorePath: string
Expand Down
Loading