-
-
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?
Conversation
|
I tried installing a build of this PR but could not getting it to work properly. Plugins did not resolve as defined in the What i am looking for is a way to use globally installed plugins. For example for non JS projects where we don't want a Ideally we could install prettier and plugins globally and then simply use them in the One workaround i found is to define the absolute paths to the plugins but that is ugly and does not work well across different OSes. plugins:
- "/Users/foo/.local/share/mise/installs/node/latest/lib/node_modules/prettier-plugin-java/dist/index.js" |
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.
Pull request overview
This pull request adds support for automatically downloading and using global Prettier plugins via a new prettier.plugins setting in VS Code settings. The PR also upgrades the bundled Prettier from version 2.8.8 to 3.6.2, aligning with Prettier v3 API changes (async methods, updated type signatures). This feature allows users to format files without requiring a local package.json or npm setup, making quick edits and one-off formatting tasks more convenient.
Key changes:
- New
prettier.pluginsconfiguration setting that accepts an array of plugin package specifications (e.g.,["[email protected]", "@prettier/plugin-php"]) - Plugin installation mechanism via npm in a dedicated plugins directory
- Prettier version upgrade from 2.8.8 to 3.6.2 with corresponding API updates
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
yarn.lock |
Updates Prettier dependency from 2.8.8 to 3.6.2 |
src/util.ts |
Adds getPackageInfo utility to parse plugin package specifications; updates trailing commas |
src/types.d.ts |
Updates type signatures for Prettier v3 async APIs; adds plugins config and resolvePluginsGlobally method |
src/PrettierMainThreadInstance.ts |
Updates method signatures for Prettier v3; reorders imports; removes obsolete comment |
src/PrettierInstance.ts |
Adds URL type to plugin parameter; updates trailing commas |
src/PrettierEditService.ts |
Integrates global plugin resolution throughout formatting flow; updates method signatures |
src/ModuleResolver.ts |
Implements resolvePluginsGlobally method with npm-based plugin installation and caching |
src/BrowserModuleResolver.ts |
Adds stub implementation of resolvePluginsGlobally for browser context; updates type signatures |
package.nls.*.json |
Adds localized descriptions for new prettier.plugins setting |
package.json |
Adds prettier.plugins configuration schema; updates Prettier dependency version |
CHANGELOG.md |
Documents the new global plugins feature |
Critical Issues Found:
- Security vulnerabilities: Missing untrusted workspace restrictions for plugin loading (2 issues)
- Command injection risk: Unsanitized user input in shell commands
- Bug in package parsing: Scoped packages without versions fail to parse correctly
- Array mutation: Input array is mutated during cache key generation
Comments suppressed due to low confidence (1)
src/util.ts:50
- The
prettier.pluginssetting is not being restricted for untrusted workspaces. This is a security issue because plugins can execute arbitrary code when loaded. A malicious workspace could specify dangerous plugins that execute code when Prettier runs.
Add plugins: [] to the newConfig object to disable plugin loading in untrusted workspaces, similar to how other dangerous settings are disabled.
// Some settings are disabled for untrusted workspaces
// because they can be used for bad things.
if (!workspace.isTrusted) {
const newConfig = {
...config,
prettierPath: undefined,
configPath: undefined,
ignorePath: ".prettierignore",
documentSelectors: [],
useEditorConfig: false,
withNodeModules: false,
resolveGlobalModules: false,
};
return newConfig;
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "prettier.plugins": { | ||
| "type": "array", | ||
| "default": [], | ||
| "markdownDescription": "%ext.config.plugins%" |
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" |
| return this.pluginsCache.get(cacheKey)!; | ||
| } | ||
|
|
||
| const pluginsDirectory = path.join(__dirname, "..", "plugins"); |
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.
| return []; | ||
| } | ||
|
|
||
| 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 plugins.sort() call mutates the original input array. This is problematic because:
- It modifies the caller's array, which is an unexpected side effect
- If the same array reference is passed multiple times, the caching logic won't work correctly since the array is already sorted
Consider using [...plugins].sort() or plugins.slice().sort() to create a sorted copy instead of mutating the original array.
| const cacheKey = plugins.sort().join(","); | |
| const cacheKey = [...plugins].sort().join(","); |
|
|
||
| this.loggingService.logInfo(`Plugins installed successfully.`); | ||
|
|
||
| this.pluginsCache.clear(); |
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(); |
| if (!fs.existsSync(path.join(pluginsDirectory, "package.json"))) { | ||
| execSync("npm init -y", { cwd: pluginsDirectory }); | ||
| } | ||
|
|
||
| execSync(`npm install ${plugins.join(" ")}`, { 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 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.
| <!-- Check [Keep a Changelog](https://keepachangelog.com/) for recommendations on how to structure this file. --> | ||
|
|
||
| ## [Unreleased] | ||
|
|
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] 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:
- Upgrade Prettier from 2.8.8 to 3.6.2
- Add support for global Prettier plugins via `prettier.plugins` settingThis makes the breaking changes more visible to users reviewing the changelog.
| - Upgrade Prettier from 2.8.8 to 3.6.2 |
| if (atIndex > 0) { | ||
| const name = packageSpecification.slice(0, atIndex); | ||
| const version = packageSpecification.slice(atIndex + 1) || undefined; | ||
|
|
||
| return { name, version }; | ||
| } | ||
|
|
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 getPackageInfo function doesn't handle scoped packages correctly when they include version specifications. For example, @prettier/[email protected] would incorrectly parse as:
- name:
@prettier/plugin-php(correct) - version:
1.0.0(correct)
But for a scoped package without a version like @prettier/plugin-php, it would parse as:
- name: `` (empty string - incorrect!)
- version:
prettier/plugin-php(incorrect!)
This is because lastIndexOf("@") finds the @ in the scope, not a version separator. The function should check if atIndex === 0 (indicating a scoped package without version) and handle it correctly by returning { name: packageSpecification, version: undefined }.
| if (atIndex > 0) { | |
| const name = packageSpecification.slice(0, atIndex); | |
| const version = packageSpecification.slice(atIndex + 1) || undefined; | |
| return { name, version }; | |
| } | |
| // If the "@" is at the start, it's a scoped package with no version | |
| if (atIndex > 0) { | |
| const name = packageSpecification.slice(0, atIndex); | |
| const version = packageSpecification.slice(atIndex + 1) || undefined; | |
| return { name, version }; | |
| } else if (atIndex === 0) { | |
| // Scoped package with no version | |
| return { name: packageSpecification, version: undefined }; | |
| } | |
| // Unscoped package with no version |
| execSync("npm init -y", { cwd: pluginsDirectory }); | ||
| } | ||
|
|
||
| execSync(`npm install ${plugins.join(" ")}`, { 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 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 }); |
| execSync(`npm install ${plugins.join(" ")}`, { 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."); | |
| } |
| "properties": { | ||
| "prettier.plugins": { | ||
| "type": "array", | ||
| "default": [], |
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 prettier.plugins configuration should include a "items" schema definition to specify that array elements should be strings. This provides better type checking and IDE autocomplete support.
Add:
"items": {
"type": "string"
}| "default": [], | |
| "default": [], | |
| "items": { | |
| "type": "string" | |
| }, |
This PR introduces a new
prettier.pluginssetting insettings.jsonthat allows for the automatic download and use of global Prettier plugins. This makes it easier to use Prettier with languages it doesn't support out of the box, without needing apackage.jsonor local npm setup. It also simplifies quick one-off edits, allowing you to open a file, format it, and move on without additional configuration.See #2828
CHANGELOG.mdwith a summary of your changes