-
-
Notifications
You must be signed in to change notification settings - Fork 955
feat(biome_js_analyze): add bundleDependencies option to NoUndeclaredDependencies rule
#9170
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: next
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 |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| "@biomejs/biome": minor | ||
| --- | ||
|
|
||
| Added `bundleDependencies` option to [NoUndeclaredDependencies](https://biomejs.dev/linter/rules/no-undeclared-dependencies) rule. | ||
|
|
||
| This rule now supports imports of packages that are defined only in `bundleDependencies` array. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,8 @@ pub struct PackageJson { | |
| pub dev_dependencies: Dependencies, | ||
| pub peer_dependencies: Dependencies, | ||
| pub optional_dependencies: Dependencies, | ||
| pub bundle_dependencies: BundleDependencies, | ||
| pub bundled_dependencies: BundleDependencies, | ||
ematipico marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| pub license: Option<(Box<str>, TextRange)>, | ||
|
|
||
| pub author: Option<Box<str>>, | ||
|
|
@@ -191,6 +193,66 @@ impl Dependencies { | |
| } | ||
| } | ||
|
|
||
| #[derive(Debug, Default, Clone)] | ||
| pub struct BundleDependencies(pub Box<[Box<str>]>); | ||
|
|
||
| /// The "bundleDependencies" field is usually an array of package names (not a map like the other dependencies fields). | ||
| /// It can also be a boolean (`true` to mean “bundle everything”). | ||
| impl Deserializable for BundleDependencies { | ||
| fn deserialize( | ||
| ctx: &mut impl DeserializationContext, | ||
| value: &impl DeserializableValue, | ||
| name: &str, | ||
| ) -> Option<Self> { | ||
| struct Visitor; | ||
|
|
||
| impl DeserializationVisitor for Visitor { | ||
| type Output = BundleDependencies; | ||
|
|
||
| const EXPECTED_TYPE: DeserializableTypes = | ||
| DeserializableTypes::ARRAY.union(DeserializableTypes::BOOL); | ||
|
|
||
| fn visit_array( | ||
| self, | ||
| ctx: &mut impl DeserializationContext, | ||
| items: impl ExactSizeIterator<Item = Option<impl DeserializableValue>>, | ||
| _range: TextRange, | ||
| name: &str, | ||
| ) -> Option<Self::Output> { | ||
| let values = items | ||
| .filter_map(|item| { | ||
| let item = item?; | ||
| Deserializable::deserialize(ctx, &item, name) | ||
| }) | ||
| .collect::<Vec<Box<str>>>(); | ||
|
|
||
| Some(BundleDependencies(values.into_boxed_slice())) | ||
| } | ||
|
|
||
| fn visit_bool( | ||
| self, | ||
| _ctx: &mut impl DeserializationContext, | ||
| _value: bool, | ||
| _range: TextRange, | ||
| _name: &str, | ||
| ) -> Option<Self::Output> { | ||
| // Allow boolean value and treat it as “no explicit list”. | ||
| Some(BundleDependencies::default()) | ||
| } | ||
| } | ||
|
|
||
| value.deserialize(ctx, Visitor, name) | ||
| } | ||
| } | ||
|
|
||
| impl BundleDependencies { | ||
| pub fn contains(&self, specifier: &str) -> bool { | ||
| self.0 | ||
| .iter() | ||
| .any(|dependency| dependency.as_ref() == specifier) | ||
| } | ||
| } | ||
|
Comment on lines
+196
to
+254
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
npm supports a boolean value: This is a known edge case; a full fix would require knowing all 💡 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
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, I am aware of this but handling this case is rather not needed because all used packages must be defined in
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
sure, you are right, I will handle that in the
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. okay, to support 2 different value types I used similar approach as in |
||
|
|
||
| #[derive(Debug, Clone, Eq, PartialEq)] | ||
| pub enum Version { | ||
| SemVer(Range), | ||
|
|
@@ -286,6 +348,16 @@ impl DeserializationVisitor for PackageJsonVisitor { | |
| result.optional_dependencies = deps; | ||
| } | ||
| } | ||
| "bundleDependencies" => { | ||
| if let Some(deps) = Deserializable::deserialize(ctx, &value, &key_text) { | ||
| result.bundle_dependencies = deps; | ||
| } | ||
| } | ||
| "bundledDependencies" => { | ||
| if let Some(deps) = Deserializable::deserialize(ctx, &value, &key_text) { | ||
| result.bundled_dependencies = deps; | ||
| } | ||
| } | ||
| "type" => { | ||
| result.r#type = Deserializable::deserialize(ctx, &value, &key_text); | ||
| } | ||
|
|
@@ -392,4 +464,23 @@ mod tests { | |
|
|
||
| assert_eq!(result, Ok(Version::Literal("~0.x.0".to_string()))); | ||
| } | ||
|
|
||
| #[test] | ||
| fn parse_package_json_bundle_dependencies_field_with_bool() { | ||
| let deserialized = deserialize_from_json_str::<PackageJson>( | ||
| r#"{ | ||
| "name": "@shared/format", | ||
| "bundleDependencies": true, | ||
| "bundledDependencies": false | ||
| }"#, | ||
| JsonParserOptions::default(), | ||
| "", | ||
| ); | ||
| let (package_json, errors) = deserialized.consume(); | ||
| assert!(errors.is_empty()); | ||
|
|
||
| let package_json = package_json.expect("parsing must have succeeded"); | ||
| assert!(package_json.bundle_dependencies.0.is_empty()); | ||
| assert!(package_json.bundled_dependencies.0.is_empty()); | ||
| } | ||
| } | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
bundleDependencies: truehas real npm semantics — worth a coverage note.Per the npm spec,
"bundleDependencies": truemeans "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 istrue, 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.tsinvalid test file explaining why booleans are used here, since future contributors may otherwise wonder why not an array.🤖 Prompt for AI Agents