-
-
Notifications
You must be signed in to change notification settings - Fork 501
Add support for configuring global Prettier plugins via prettier.plugins #3782
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -127,7 +127,7 @@ | |||||||||||
| }, | ||||||||||||
| "dependencies": { | ||||||||||||
| "find-up": "5.0.0", | ||||||||||||
| "prettier": "^2.8.8", | ||||||||||||
| "prettier": "^3.6.2", | ||||||||||||
| "resolve": "^1.22.8", | ||||||||||||
| "semver": "^7.6.3", | ||||||||||||
| "vscode-nls": "^5.2.0" | ||||||||||||
|
|
@@ -154,6 +154,11 @@ | |||||||||||
| "type": "object", | ||||||||||||
| "title": "%ext.config.title%", | ||||||||||||
| "properties": { | ||||||||||||
| "prettier.plugins": { | ||||||||||||
| "type": "array", | ||||||||||||
| "default": [], | ||||||||||||
|
||||||||||||
| "default": [], | |
| "default": [], | |
| "items": { | |
| "type": "string" | |
| }, |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider adding a "scope" property to the prettier.plugins configuration. Based on other settings in this file, global plugin configuration should likely use "scope": "window" to apply at the workspace level rather than per-resource, similar to prettier.disableLanguages and prettier.documentSelectors.
| "markdownDescription": "%ext.config.plugins%" | |
| "markdownDescription": "%ext.config.plugins%", | |
| "scope": "window" |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -16,18 +16,23 @@ import { | |||||||||||||||||||||||||||||||||
| UNTRUSTED_WORKSPACE_USING_BUNDLED_PRETTIER, | ||||||||||||||||||||||||||||||||||
| USING_BUNDLED_PRETTIER, | ||||||||||||||||||||||||||||||||||
| } from "./message"; | ||||||||||||||||||||||||||||||||||
| import { loadNodeModule, resolveConfigPlugins } from "./ModuleLoader"; | ||||||||||||||||||||||||||||||||||
| import { PrettierInstance } from "./PrettierInstance"; | ||||||||||||||||||||||||||||||||||
| import { PrettierMainThreadInstance } from "./PrettierMainThreadInstance"; | ||||||||||||||||||||||||||||||||||
| import { PrettierWorkerInstance } from "./PrettierWorkerInstance"; | ||||||||||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||||||||||
| ModuleResolverInterface, | ||||||||||||||||||||||||||||||||||
| PackageManagers, | ||||||||||||||||||||||||||||||||||
| PrettierOptions, | ||||||||||||||||||||||||||||||||||
| PrettierResolveConfigOptions, | ||||||||||||||||||||||||||||||||||
| PrettierVSCodeConfig, | ||||||||||||||||||||||||||||||||||
| } from "./types"; | ||||||||||||||||||||||||||||||||||
| import { getConfig, getWorkspaceRelativePath, isAboveV3 } from "./util"; | ||||||||||||||||||||||||||||||||||
| import { PrettierWorkerInstance } from "./PrettierWorkerInstance"; | ||||||||||||||||||||||||||||||||||
| import { PrettierInstance } from "./PrettierInstance"; | ||||||||||||||||||||||||||||||||||
| import { PrettierMainThreadInstance } from "./PrettierMainThreadInstance"; | ||||||||||||||||||||||||||||||||||
| import { loadNodeModule, resolveConfigPlugins } from "./ModuleLoader"; | ||||||||||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||||||||||
| getConfig, | ||||||||||||||||||||||||||||||||||
| getPackageInfo, | ||||||||||||||||||||||||||||||||||
| getWorkspaceRelativePath, | ||||||||||||||||||||||||||||||||||
| isAboveV3, | ||||||||||||||||||||||||||||||||||
| } from "./util"; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| const minPrettierVersion = "1.13.0"; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
@@ -57,6 +62,8 @@ const fsStatSyncWorkaround = ( | |||||||||||||||||||||||||||||||||
| // @ts-expect-error Workaround for https://github.com/prettier/prettier-vscode/issues/3020 | ||||||||||||||||||||||||||||||||||
| fs.statSync = fsStatSyncWorkaround; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| declare const __non_webpack_require__: NodeRequire; | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| const globalPaths: { | ||||||||||||||||||||||||||||||||||
| [key: string]: { cache: string | undefined; get(): string | undefined }; | ||||||||||||||||||||||||||||||||||
| } = { | ||||||||||||||||||||||||||||||||||
|
|
@@ -95,6 +102,7 @@ function globalPathGet(packageManager: PackageManagers): string | undefined { | |||||||||||||||||||||||||||||||||
| export class ModuleResolver implements ModuleResolverInterface { | ||||||||||||||||||||||||||||||||||
| private findPkgCache: Map<string, string>; | ||||||||||||||||||||||||||||||||||
| private ignorePathCache = new Map<string, string>(); | ||||||||||||||||||||||||||||||||||
| private pluginsCache = new Map<string, string[]>(); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| private path2Module = new Map<string, PrettierInstance>(); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
@@ -121,7 +129,7 @@ export class ModuleResolver implements ModuleResolverInterface { | |||||||||||||||||||||||||||||||||
| return pkgFilePath; | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||
| { cwd } | ||||||||||||||||||||||||||||||||||
| { cwd }, | ||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| if (!packageJsonPath) { | ||||||||||||||||||||||||||||||||||
|
|
@@ -344,6 +352,52 @@ export class ModuleResolver implements ModuleResolverInterface { | |||||||||||||||||||||||||||||||||
| return fileName; | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| public resolvePluginsGlobally(plugins: string[]): string[] { | ||||||||||||||||||||||||||||||||||
| if (plugins.length === 0) { | ||||||||||||||||||||||||||||||||||
| return []; | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| const cacheKey = plugins.sort().join(","); | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
| const cacheKey = plugins.sort().join(","); | |
| const cacheKey = [...plugins].sort().join(","); |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pluginsDirectory path uses __dirname which refers to the compiled output location. In a webpack-bundled extension, this could be unpredictable. Consider using the extension's global storage path via VS Code's ExtensionContext.globalStorageUri instead, which is the recommended location for extension-managed data.
This would require passing the storage path through the constructor, but would make the extension more robust and follow VS Code best practices.
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The plugin installation happens synchronously on every call when the cache is empty, which will block the extension thread. This could cause VS Code to become unresponsive, especially if:
- Multiple plugins need to be installed
- The npm registry is slow
- Large plugins are being downloaded
Consider making this operation asynchronous or showing a progress indicator to the user. The function signature should change to return Promise<string[]> and use async/await or spawn the npm process asynchronously.
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The npm install command is vulnerable to command injection. Plugin names from user configuration are directly interpolated into a shell command without sanitization. A malicious plugin name like ; rm -rf / or $(malicious-command) could execute arbitrary commands.
Consider using execSync with an array of arguments via the shell: false option, or properly escape/validate the plugin names before passing them to the shell. For example:
const args = ['install', ...plugins];
execSync(`npm ${args.map(arg => JSON.stringify(arg)).join(' ')}`, { cwd: pluginsDirectory });Or better yet, use a programmatic API if available, or validate that each plugin name matches a safe pattern (e.g., /^[@a-z0-9-_/]+$/i).
| execSync(`npm install ${plugins.join(" ")}`, { cwd: pluginsDirectory }); | |
| execSync('npm', ['install', ...plugins], { cwd: pluginsDirectory }); |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The plugin installation logic doesn't check if the plugins are already installed before running npm install. This means every time VS Code starts or the configuration changes, the extension will re-run npm install even if the plugins are already present.
Consider checking if the plugins are already installed in node_modules before running npm install. This would significantly speed up repeated calls and reduce unnecessary network requests.
| execSync(`npm install ${plugins.join(" ")}`, { cwd: pluginsDirectory }); | |
| // Check which plugins are missing from node_modules | |
| const missingPlugins = plugins.filter((plugin) => { | |
| const pluginPackageName = getPackageInfo(plugin).name; | |
| const pluginPath = path.join(pluginsDirectory, "node_modules", pluginPackageName); | |
| return !fs.existsSync(pluginPath); | |
| }); | |
| if (missingPlugins.length > 0) { | |
| this.loggingService.logInfo( | |
| `Installing missing plugins: ${missingPlugins.join(", ")}` | |
| ); | |
| execSync(`npm install ${missingPlugins.join(" ")}`, { cwd: pluginsDirectory }); | |
| } else { | |
| this.loggingService.logInfo("All plugins already installed, skipping npm install."); | |
| } |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pluginsCache.clear() call before setting the new cache entry defeats the purpose of caching. If a user has multiple plugin configurations (e.g., ["plugin-a"] and ["plugin-b"]), installing one set will clear the cache for all others, forcing reinstallation.
Remove the clear() call and just set the specific cache key:
this.pluginsCache.set(cacheKey, resolvedPlugins);The cache should only be cleared when the extension is disposed or reloaded.
| this.pluginsCache.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The CHANGELOG entry should include more context about the Prettier version upgrade from 2.8.8 to 3.6.2, as this is a major version change that could affect users. Consider adding a separate entry like:
This makes the breaking changes more visible to users reviewing the changelog.