fix: resolve plugin paths from extended configs#9233
fix: resolve plugin paths from extended configs#9233soconnor-seeq wants to merge 1 commit intobiomejs:mainfrom
Conversation
🦋 Changeset detectedLatest commit: bc8b25a The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThis patch resolves plugin paths in extended configuration files relative to their extended config location rather than the root config directory. A new private helper function normalises plugin paths across a configuration and its overrides, consolidating previously inline logic. The function is invoked when processing extended configurations prior to final assembly, and test coverage has been expanded to verify normalisation in extended config scenarios. Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_service/src/workspace.tests.rs (1)
566-646: Optional: make the assertion order‑agnostic. It’ll be a touch less brittle if diagnostic ordering changes later.♻️ Possible tweak
- let rendered = print_diagnostic_to_string(&Error::from(result.diagnostics[0].clone())); - assert!(rendered.contains("Prefer object spread instead of `Object.assign()`")); + let rendered_any = result.diagnostics.iter().any(|diag| { + print_diagnostic_to_string(&Error::from(diag.clone())) + .contains("Prefer object spread instead of `Object.assign()`") + }); + assert!(rendered_any);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_service/src/workspace.tests.rs` around lines 566 - 646, The test plugins_support_package_specifiers_via_package_json currently assumes the expected diagnostic is at result.diagnostics[0], which is brittle; update the final assertion to be order‑agnostic by checking that any diagnostic in result.diagnostics contains the message ("Prefer object spread instead of `Object.assign()`") instead of indexing the first element—use result.diagnostics.iter().any(...) or equivalent over each diagnostic (render/convert each diagnostic to string as done with print_diagnostic_to_string/Error::from) and assert that returns true while keeping the existing checks like assert_eq!(result.errors, 0) and that diagnostics is not empty.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/biome_service/src/workspace.tests.rs`:
- Around line 566-646: The test
plugins_support_package_specifiers_via_package_json currently assumes the
expected diagnostic is at result.diagnostics[0], which is brittle; update the
final assertion to be order‑agnostic by checking that any diagnostic in
result.diagnostics contains the message ("Prefer object spread instead of
`Object.assign()`") instead of indexing the first element—use
result.diagnostics.iter().any(...) or equivalent over each diagnostic
(render/convert each diagnostic to string as done with
print_diagnostic_to_string/Error::from) and assert that returns true while
keeping the existing checks like assert_eq!(result.errors, 0) and that
diagnostics is not empty.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/fix-extended-plugin-paths.mdcrates/biome_service/src/configuration.rscrates/biome_service/src/workspace.tests.rscrates/biome_service/src/workspace/server.rs
|
What bug are we fixing? Is there one already? |
But that's how it should be, because it works the same as the rest of the other resolution paths. Is there a specific reason why we should diverge from that? |
Oh does there need to be a bug / issue created before a PR?
The current implementation makes it impossible for me to make a common tools package w/ a standard monorepo-biome-config that can be This change makes it so that relative URLs are resolved relative to the |
|
I don't know why I didn't come across it before but re-searching issues / discussion it looks like this has come up before. |
|
I'm willing to accept this PR, considering we have multiple reports, however I think we should also update the documentation of the plugins and the documentation of the configuration, because we're adding an "exception to the rule". People usually are in charge of documentation. Docs are in |
Can do! I've created the website documentation change PR. |
soconnor-seeq
force-pushed
the
fix/extended-plugin-paths
branch
from
3339791 to
e9d5a9f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/biome_service/src/workspace/server.rs (3)
823-858: Consider adding rustdoc forresolve_plugin_specifier.This method has nuanced behaviour (handling absolute paths, relative specifiers,
@-scoped packages, and biome-manifest special-casing). A brief rustdoc would help future maintainers understand the resolution semantics without diving into the implementation.As per coding guidelines: "Use inline rustdoc documentation for rules, assists, and their options."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_service/src/workspace/server.rs` around lines 823 - 858, Add a concise rustdoc for the resolve_plugin_specifier method explaining its resolution rules: that it returns absolute/relative specifiers as-is, treats non-@ specifiers as plain paths, uses ResolveOptions (RESOLVE_OPTIONS) for `@-scoped` packages with condition names ("biome","default"), default file "biome-manifest" and extensions ("jsonc","grit","js","mjs"), and that it special-cases "biome-manifest.jsonc" to return the parent directory; mention the error behavior (returns PluginDiagnostic via PluginDiagnostic::cant_resolve on resolve failures). Reference the function name resolve_plugin_specifier and the RESOLVE_OPTIONS constant in the doc.
796-818: Consider caching resolved paths to avoid double resolution.The plugin specifiers are resolved twice: once in
load_pluginsand again here for cache lookup. While this works correctly, if plugin resolution becomes more expensive (e.g., with complex package.json lookups), you might want to store the original-to-resolved mapping.That said, for the current scope this is perfectly acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_service/src/workspace/server.rs` around lines 796 - 818, The code double-resolves plugin specifiers (once in load_plugins and again here) — introduce and use a short-lived mapping from original specifier to resolved path so resolution happens only once: when resolving plugins (where resolve_plugin_specifier is currently called), store the mapping (e.g., HashMap<String, String> keyed by the PluginConfiguration input) and then update this code path to look up resolved paths from that map instead of calling resolve_plugin_specifier again; ensure the resolved collection you pass into cache.get_analyzer_plugins is built from that map (maintain the existing Plugins wrapper and continue to push PluginConfiguration::Path(resolved) into resolved.
847-857: Edge case:parent()returningNoneyields the file path, not a directory.If the resolved path is somehow just
biome-manifest.jsoncwithout any parent (unlikely but possible),unwrap_or(resolved.as_path())returns the file path itself rather than a meaningful directory.Consider making this explicit or adding a debug assertion:
🔧 Suggested fix
if resolved .file_name() .is_some_and(|name| name == "biome-manifest.jsonc") { - Ok(resolved - .parent() - .unwrap_or(resolved.as_path()) - .to_path_buf()) + Ok(resolved.parent().map_or_else( + || { + // This shouldn't happen in practice - a manifest file should always have a parent + debug_assert!(false, "biome-manifest.jsonc resolved without parent directory"); + resolved.to_path_buf() + }, + |parent| parent.to_path_buf(), + )) } else { Ok(resolved) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_service/src/workspace/server.rs` around lines 847 - 857, When handling the case where resolved.file_name() == "biome-manifest.jsonc", make the parent-none case explicit: instead of using unwrap_or(resolved.as_path()) (which returns the file path), check resolved.parent() and if Some(parent) return parent.to_path_buf(), otherwise add a debug_assert!(resolved.parent().is_some()) and return a safe fallback directory (for example std::env::current_dir() or an appropriate error) so the function always returns a directory path rather than the file itself; update the branch that references resolved, file_name(), parent(), unwrap_or(), and to_path_buf() accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/biome_service/src/workspace/server.rs`:
- Around line 823-858: Add a concise rustdoc for the resolve_plugin_specifier
method explaining its resolution rules: that it returns absolute/relative
specifiers as-is, treats non-@ specifiers as plain paths, uses ResolveOptions
(RESOLVE_OPTIONS) for `@-scoped` packages with condition names
("biome","default"), default file "biome-manifest" and extensions
("jsonc","grit","js","mjs"), and that it special-cases "biome-manifest.jsonc" to
return the parent directory; mention the error behavior (returns
PluginDiagnostic via PluginDiagnostic::cant_resolve on resolve failures).
Reference the function name resolve_plugin_specifier and the RESOLVE_OPTIONS
constant in the doc.
- Around line 796-818: The code double-resolves plugin specifiers (once in
load_plugins and again here) — introduce and use a short-lived mapping from
original specifier to resolved path so resolution happens only once: when
resolving plugins (where resolve_plugin_specifier is currently called), store
the mapping (e.g., HashMap<String, String> keyed by the PluginConfiguration
input) and then update this code path to look up resolved paths from that map
instead of calling resolve_plugin_specifier again; ensure the resolved
collection you pass into cache.get_analyzer_plugins is built from that map
(maintain the existing Plugins wrapper and continue to push
PluginConfiguration::Path(resolved) into resolved.
- Around line 847-857: When handling the case where resolved.file_name() ==
"biome-manifest.jsonc", make the parent-none case explicit: instead of using
unwrap_or(resolved.as_path()) (which returns the file path), check
resolved.parent() and if Some(parent) return parent.to_path_buf(), otherwise add
a debug_assert!(resolved.parent().is_some()) and return a safe fallback
directory (for example std::env::current_dir() or an appropriate error) so the
function always returns a directory path rather than the file itself; update the
branch that references resolved, file_name(), parent(), unwrap_or(), and
to_path_buf() accordingly.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/fix-extended-plugin-paths.mdcrates/biome_service/src/configuration.rscrates/biome_service/src/workspace.tests.rscrates/biome_service/src/workspace/server.rs
✅ Files skipped from review due to trivial changes (1)
- .changeset/fix-extended-plugin-paths.md
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/biome_service/src/workspace.tests.rs
- crates/biome_service/src/configuration.rs
|
@ematipico (sorry for the discussion / noise on this PR but) I am not wondering if this PR's approach is correct. The PR #8524 made it so root-configs resolve plugins relative to the root-config, this PR makes it so Also it looks like the |
soconnor-seeq
force-pushed
the
fix/extended-plugin-paths
branch
from
e9d5a9f to
bc8b25a
Compare
2 participants
Note
(from the human) I don’t know Rust at all & all these changes were implemented via Codex with my guidance. I've built the binary and tested it locally using my monorepo as a test case (see example layout below).
Summary
Fixes plugin path resolution for plugins listed in extended config files and adds
@-specifier support viapackage.jsonresolution, while preserving legacy non‑@behavior.Context
Previously, plugin paths from extended configs were normalized only after all configs were merged, so relative plugin paths were resolved against the entry config rather than the config file where they were declared. This change normalizes plugin paths at load time so extended configs resolve their own relative plugin paths correctly.
My use case is a PNPM monorepo with a shared tools package.
extendsresolves fine, but custom Grit plugins declared in the shared config were not resolved relative to the common/tools package.Example layout
package-a/biome.json:{ "extends": ["@my-scope/package-tools/biome-common.json"] }package-b/biome.json:{ "extends": ["@my-scope/package-tools/biome-common.json"] }package-tools/biome-common.json:{ "plugins": ["./plugins/plugin-a.grit"] }package-tools/package.json(PNPM workspace structure, abbreviated):{ "name": "@my-scope/package-tools", "private": true }package-a/package.json/package-b/package.json:{ "name": "@my-scope/package-a", "private": true }Test Plan
cargo test -p biome_service should_normalize_plugin_paths_in_extended_configurationcargo test -p biome_service plugins_support_package_specifiers_via_package_jsoncargo test -p biome_cli --test main -- tests/ config.rs::extends_config_extends_external_plugins_without_relative_paths tests/ config.rs::extends_config_extends_plugins_with_relative_paths tests/ config.rs::extends_config_extends_plugins_only_from_basecargo test -p biome_lsp --libDocs
Not applicable.
Future Enhancement
Note that the changes in this PR are 100% backward‑compatible with the existing plugin file resolution (the current
"extend"-ed plugins paths are broken ATM). Any non‑@plugin specifiers still use the current behaviour (so a simple-path like"plugins": ["foo.grit"]resolves as before — relative to the entry config — only./or../-prefixes are treated as explicit relative paths to the declaring config).We could allow a small-ish "breaking" change to allow unscoped package names to resolve via
package.jsonornode_modules. This could be done via an explicit./or../(or sniff the file system,package.json, etc) to distinguish file paths from package names. The benefit here would be to make it more obvious between: a) a relative plugin./or../-prefixed path or b) a "external" package (either via workspaces or npmjs.com).