feat(biome_js_analyze): add bundleDependencies option to NoUndeclaredDependencies rule#9170
feat(biome_js_analyze): add bundleDependencies option to NoUndeclaredDependencies rule#9170mdrobny wants to merge 2 commits intobiomejs:nextfrom
bundleDependencies option to NoUndeclaredDependencies rule#9170Conversation
🦋 Changeset detectedLatest commit: a1535fb The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 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 |
github-actions
bot
added
A-Project
WalkthroughAdds bundleDependencies and bundledDependencies support across the NoUndeclaredDependencies lint: package.json parsing now reads those fields, ManifestServices gains an is_bundle_dependency method, rule options accept bundle dependency availability, RuleState and diagnostics propagate the new availability flag, and tests cover valid/invalid cases. 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.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/biome_js_analyze/src/lint/correctness/no_undeclared_dependencies.rs (1)
259-267:⚠️ Potential issue | 🟠 MajorAdd snapshot test for
bundleDependencies: falseoption.The code path for bundleDependencies (lines 259–267) is not exercised by the test suite. The current tests cover
devDependencies: falseand valid bundleDependencies, but there's no invalid test case that disables bundleDependencies and verifies imports trigger a diagnostic. Following the pattern ofinvalid.options.json, add a test variant with"bundleDependencies": falsein its options configuration and an import from a bundled package in the corresponding.jsfile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/src/lint/correctness/no_undeclared_dependencies.rs` around lines 259 - 267, Add a snapshot test exercising the bundleDependencies=false path: create a test variant mirroring the existing invalid.options.json pattern but with "bundleDependencies": false in the options and a corresponding .js test file that imports a package that would be treated as a bundle dependency so the rule emits a diagnostic; this will exercise the code path checking ctx.is_bundle_dependency(package_name) and the is_bundle_dependency_available logic in no_undeclared_dependencies.rs so the bundleDependencies branch produces a failing diagnostic as expected.
🧹 Nitpick comments (1)
crates/biome_package/src/node_js_package/package_json.rs (1)
36-38:bundled_dependenciesfield is missing a doc comment.The preceding
bundle_dependenciesfield has one;bundled_dependenciesdoes not. Worth adding a short note (e.g.,"bundledDependencies"is the legacy alias for"bundleDependencies") so the struct is self-documenting.📝 Suggested doc comment
pub bundle_dependencies: BundleDependencies, +/// The "bundledDependencies" field is the legacy alias of "bundleDependencies" and is treated identically. pub bundled_dependencies: BundleDependencies,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_package/src/node_js_package/package_json.rs` around lines 36 - 38, Add a doc comment for the bundled_dependencies field explaining it is the legacy alias for "bundleDependencies" and that it is an array of package names (same type as bundle_dependencies); update the struct so pub bundled_dependencies: BundleDependencies has a short comment like `"bundledDependencies" is the legacy alias for "bundleDependencies" (an array of package names)` to make the struct self-documenting and mirror the existing comment on bundle_dependencies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_js_analyze/src/lint/correctness/no_undeclared_dependencies.rs`:
- Around line 91-92: There is a trailing space at the end of the doc comment
line that starts "In this example, only test files can use dependencies in the
`devDependencies` section." in no_undeclared_dependencies.rs; remove the
trailing whitespace from that line (update the doc comment text) and re-run the
formatter/checker (just f) before committing to ensure no other whitespace
issues remain.
In `@crates/biome_package/src/node_js_package/package_json.rs`:
- Around line 197-217: The BundleDependencies.deserialize currently treats
non-array values (notably the npm shorthand "bundleDependencies": true) as
absent because Vec<Box<str>>::deserialize returns None; add a concise TODO
comment inside the BundleDependencies::deserialize impl (near the let values:
Vec<Box<str>> = ... line) noting the known edge case where a boolean true means
"bundle all dependencies" and is silently ignored, and mention that a fuller fix
would require resolving the package's dependencies keys later; keep the existing
behavior but document it to avoid future confusion.
---
Outside diff comments:
In `@crates/biome_js_analyze/src/lint/correctness/no_undeclared_dependencies.rs`:
- Around line 259-267: Add a snapshot test exercising the
bundleDependencies=false path: create a test variant mirroring the existing
invalid.options.json pattern but with "bundleDependencies": false in the options
and a corresponding .js test file that imports a package that would be treated
as a bundle dependency so the rule emits a diagnostic; this will exercise the
code path checking ctx.is_bundle_dependency(package_name) and the
is_bundle_dependency_available logic in no_undeclared_dependencies.rs so the
bundleDependencies branch produces a failing diagnostic as expected.
---
Nitpick comments:
In `@crates/biome_package/src/node_js_package/package_json.rs`:
- Around line 36-38: Add a doc comment for the bundled_dependencies field
explaining it is the legacy alias for "bundleDependencies" and that it is an
array of package names (same type as bundle_dependencies); update the struct so
pub bundled_dependencies: BundleDependencies has a short comment like
`"bundledDependencies" is the legacy alias for "bundleDependencies" (an array of
package names)` to make the struct self-documenting and mirror the existing
comment on bundle_dependencies.
crates/biome_js_analyze/src/lint/correctness/no_undeclared_dependencies.rs
Outdated
Show resolved
Hide resolved
| #[derive(Debug, Default, Clone)] | ||
| pub struct BundleDependencies(pub Box<[Box<str>]>); | ||
|
|
||
| impl Deserializable for BundleDependencies { | ||
| fn deserialize( | ||
| ctx: &mut impl DeserializationContext, | ||
| value: &impl DeserializableValue, | ||
| name: &str, | ||
| ) -> Option<Self> { | ||
| let values: Vec<Box<str>> = Deserializable::deserialize(ctx, value, name)?; | ||
| Some(Self(values.into_boxed_slice())) | ||
| } | ||
| } | ||
|
|
||
| impl BundleDependencies { | ||
| pub fn contains(&self, specifier: &str) -> bool { | ||
| self.0 | ||
| .iter() | ||
| .any(|dependency| dependency.as_ref() == specifier) | ||
| } | ||
| } |
There was a problem hiding this comment.
"bundleDependencies": true is silently swallowed.
npm supports a boolean value: true bundles all dependencies, false bundles none. When the field holds true, Vec<Box<str>>::deserialize returns None, the if let Some(deps) guard is never taken, and the field stays as the default empty BundleDependencies. The rule will then report every import from that package as undeclared, producing false positives.
This is a known edge case; a full fix would require knowing all dependencies keys at that point, so it's fine to leave for a follow-up. A short // TODO comment here would prevent future confusion.
💡 Suggested TODO
impl Deserializable for BundleDependencies {
fn deserialize(
ctx: &mut impl DeserializationContext,
value: &impl DeserializableValue,
name: &str,
) -> Option<Self> {
+ // TODO: handle `"bundleDependencies": true` (bundle all deps) –
+ // that requires access to the `dependencies` map, which isn't available here.
let values: Vec<Box<str>> = Deserializable::deserialize(ctx, value, name)?;
Some(Self(values.into_boxed_slice()))
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/biome_package/src/node_js_package/package_json.rs` around lines 197 -
217, The BundleDependencies.deserialize currently treats non-array values
(notably the npm shorthand "bundleDependencies": true) as absent because
Vec<Box<str>>::deserialize returns None; add a concise TODO comment inside the
BundleDependencies::deserialize impl (near the let values: Vec<Box<str>> = ...
line) noting the known edge case where a boolean true means "bundle all
dependencies" and is silently ignored, and mention that a fuller fix would
require resolving the package's dependencies keys later; keep the existing
behavior but document it to avoid future confusion.
There was a problem hiding this comment.
yes, I am aware of this but handling this case is rather not needed because all used packages must be defined in dependencies field already so any undeclared package will be detected
There was a problem hiding this comment.
While handling the Boolean value isn't needed to the feature, Biome must understand the Boolean value too as per docs https://docs.npmjs.com/cli/v8/configuring-npm/package-json#bundledependencies
If we don't, Biome will start failing the moment it will try to deserialise Boolean values
There was a problem hiding this comment.
Biome must understand the Boolean value
sure, you are right, I will handle that in the PackageJson struct
There was a problem hiding this comment.
okay, to support 2 different value types I used similar approach as in Dependencies with DeserializationVisitor, I hope it makes sense
dyc3
left a comment
dyc3
left a comment
There was a problem hiding this comment.
I'm not sure this is correct behavior. If you only have the dependency in bundleDependencies, does the package manager pull the package when you run the install command?
ematipico
left a comment
ematipico
left a comment
There was a problem hiding this comment.
Need to address the bot comments. Plus, bundled dependencies doesn't exist, and need to be removed
| #[derive(Debug, Default, Clone)] | ||
| pub struct BundleDependencies(pub Box<[Box<str>]>); | ||
|
|
||
| impl Deserializable for BundleDependencies { | ||
| fn deserialize( | ||
| ctx: &mut impl DeserializationContext, | ||
| value: &impl DeserializableValue, | ||
| name: &str, | ||
| ) -> Option<Self> { | ||
| let values: Vec<Box<str>> = Deserializable::deserialize(ctx, value, name)?; | ||
| Some(Self(values.into_boxed_slice())) | ||
| } | ||
| } | ||
|
|
||
| impl BundleDependencies { | ||
| pub fn contains(&self, specifier: &str) -> bool { | ||
| self.0 | ||
| .iter() | ||
| .any(|dependency| dependency.as_ref() == specifier) | ||
| } | ||
| } |
There was a problem hiding this comment.
While handling the Boolean value isn't needed to the feature, Biome must understand the Boolean value too as per docs https://docs.npmjs.com/cli/v8/configuring-npm/package-json#bundledependencies
If we don't, Biome will start failing the moment it will try to deserialise Boolean values
- to support detecting imports from `bundleDependencies` - currently this rule will always throw error when dependency is defined only in `bundleDependencies` array
mdrobny
force-pushed
the
feat/bundle-dependencies
branch
from
79c0efd to
a1535fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@crates/biome_js_analyze/tests/specs/correctness/noUndeclaredDependencies/invalid.package.json`:
- Around line 8-9: The fixture uses boolean values for "bundleDependencies" and
"bundledDependencies" which could be misinterpreted by a strict implementation
as meaning "bundle all dependencies"; update the corresponding invalid test .ts
file to add a short comment explaining that the booleans are intentionally used
to ensure the parser does not crash on non-array values and that the test
deliberately does not assert "all dependencies are bundled" semantics; reference
the JSON fields "bundleDependencies", "bundledDependencies", and "dependencies"
in the comment so future contributors understand why a boolean rather than an
array is provided.
| "bundleDependencies": true, | ||
| "bundledDependencies": false |
There was a problem hiding this comment.
bundleDependencies: true has real npm semantics — worth a coverage note.
Per the npm spec, "bundleDependencies": true means "bundle all dependencies", not merely a no-op. The current fixture cleverly uses booleans to verify the parser doesn't crash on non-array values, which is good. However, there's a small coverage gap: when the field is true, a strict implementation could infer that every package listed under "dependencies" is also bundled. If the parser simply skips non-array values (the most pragmatic approach), that's fine — but it may be worth adding a brief comment in the corresponding .ts invalid test file explaining why booleans are used here, since future contributors may otherwise wonder why not an array.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@crates/biome_js_analyze/tests/specs/correctness/noUndeclaredDependencies/invalid.package.json`
around lines 8 - 9, The fixture uses boolean values for "bundleDependencies" and
"bundledDependencies" which could be misinterpreted by a strict implementation
as meaning "bundle all dependencies"; update the corresponding invalid test .ts
file to add a short comment explaining that the booleans are intentionally used
to ensure the parser does not crash on non-array values and that the test
deliberately does not assert "all dependencies are bundled" semantics; reference
the JSON fields "bundleDependencies", "bundledDependencies", and "dependencies"
in the comment so future contributors understand why a boolean rather than an
array is provided.
no, package manager will not pull those dependencies. |
|
It sounds like you should just fix those rare cases where it's not declared. That's the whole point of the rule. Is there something I'm missing? |
Unfortunately the setup is complex but it's legit so I cannot fix it 😄 monorepos come with tradeoffs and sharing code across workspaces is great but it becomes a problem for tools like linters. |
3 participants
Summary
Support detecting imports from bundleDependencies field in package.json by noUndeclaredDependencies rule
bundleDependenciesare supported by NPM, Bun, pnpm.Support alternative name
bundledDependenciesalso.Currently this rule will always throw error when dependency is defined only in
bundleDependenciesarray.My motivation is to be able to use this rule in a monorepo which includes workspace published as NPM package but using another "shared" workspace as
bundleDependency(and this shared package cannot be a regular dependency, dev, peer or optional).noDuplicateDependencies, which is related rule, already supports
bundleDependenciesI mostly manually repeated how existing rule options are implemented but since I am totally inexperienced in Rust language I was using Github Copilot agent as assistance to implement some functions correctly
Test Plan
Rule tests are passing - packages from both
bundleDependenciesandbundledDependenciesare detected correctlyDocs
I added docs about new rule options of course but I didn't want to overload them with more specific docs because it works the same as other options, I think it should be enough.