Skip to content
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

fix(linter): useNodejsImportProtocol takes in dependencies in consideration #3285

Merged
merged 2 commits into from
Jun 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ our [guidelines for writing a good changelog entry](https://github.com/biomejs/b
#### Bug fixes

- `useConsistentArrayType` and `useShorthandArrayType` now ignore `Array` in the `extends` and `implements` clauses. Fix [#3247](https://github.com/biomejs/biome/issues/3247). Contributed by @Conaclos
- Fixes [#3066](https://github.com/biomejs/biome/issues/3066) by taking into account the dependencies declared in the `package.json`. Contributed by @ematipico

## v1.8.2 (2024-06-20)

Expand Down
16 changes: 10 additions & 6 deletions crates/biome_js_analyze/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,9 +131,7 @@ where

services.insert_service(Arc::new(AriaRoles));
services.insert_service(Arc::new(AriaProperties));
if let Some(manifest) = manifest {
services.insert_service(Arc::new(manifest));
}
services.insert_service(Arc::new(manifest));
services.insert_service(source_type);
(
analyzer.run(AnalyzerContext {
Expand Down Expand Up @@ -238,6 +236,7 @@ mod tests {
use biome_diagnostics::{Diagnostic, DiagnosticExt, PrintDiagnostic, Severity};
use biome_js_parser::{parse, JsParserOptions};
use biome_js_syntax::{JsFileSource, TextRange, TextSize};
use biome_project::{Dependencies, PackageJson};
use std::slice;

use crate::lint::correctness::use_exhaustive_dependencies::{Hook, HooksOptions};
Expand All @@ -256,7 +255,7 @@ mod tests {
String::from_utf8(buffer).unwrap()
}

const SOURCE: &str = r#"<div class={`px-2 foo p-4 bar ${variable}`}/>"#;
const SOURCE: &str = r#"import buffer from "buffer"; "#;

let parsed = parse(SOURCE, JsFileSource::tsx(), JsParserOptions::default());

Expand All @@ -268,13 +267,15 @@ mod tests {
dependencies_index: Some(1),
stable_result: StableHookResult::None,
};
let rule_filter = RuleFilter::Rule("nursery", "useSortedClasses");
let rule_filter = RuleFilter::Rule("style", "useNodejsImportProtocol");

options.configuration.rules.push_rule(
RuleKey::new("nursery", "useHookAtTopLevel"),
RuleOptions::new(HooksOptions { hooks: vec![hook] }, None),
);

let mut dependencies = Dependencies::default();
dependencies.add("buffer", "latest");
analyze(
&parsed.tree(),
AnalysisFilter {
Expand All @@ -283,7 +284,10 @@ mod tests {
},
&options,
JsFileSource::tsx(),
None,
Some(PackageJson {
dependencies,
..Default::default()
}),
|signal| {
if let Some(diag) = signal.diagnostic() {
error_ranges.push(diag.location().span.unwrap());
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
use biome_analyze::{
context::RuleContext, declare_rule, ActionCategory, Ast, FixKind, Rule, RuleDiagnostic,
RuleSource,
context::RuleContext, declare_rule, ActionCategory, FixKind, Rule, RuleDiagnostic, RuleSource,
};
use biome_console::markup;
use biome_js_syntax::{inner_string_text, AnyJsImportLike, JsSyntaxKind, JsSyntaxToken};
use biome_rowan::BatchMutationExt;

use crate::services::manifest::Manifest;
use crate::{globals::is_node_builtin_module, JsRuleAction};

declare_rule! {
Expand All @@ -14,6 +14,13 @@ declare_rule! {
/// The rule marks traditional imports like `import fs from "fs";` as invalid,
/// suggesting the format `import fs from "node:fs";` instead.
///
/// The rule also isn't triggered if there are dependencies declared in the `package.json` that match
/// the name of a built-in Node.js module.
///
/// :::caution
/// The rule doesn't support dependencies installed inside a monorepo.
/// :::
///
/// ## Examples
///
/// ### Invalid
Expand Down Expand Up @@ -51,7 +58,7 @@ declare_rule! {
}

impl Rule for UseNodejsImportProtocol {
type Query = Ast<AnyJsImportLike>;
type Query = Manifest<AnyJsImportLike>;
type State = JsSyntaxToken;
type Signals = Option<Self::State>;
type Options = ();
Expand All @@ -62,7 +69,15 @@ impl Rule for UseNodejsImportProtocol {
return None;
}
let module_name = node.module_name_token()?;
is_node_module_without_protocol(&inner_string_text(&module_name)).then_some(module_name)
let module_name_trimmed = inner_string_text(&module_name);
if ctx.is_dependency(&module_name_trimmed)
|| ctx.is_dev_dependency(&module_name_trimmed)
|| ctx.is_peer_dependency(&module_name_trimmed)
|| ctx.is_optional_dependency(&module_name_trimmed)
{
return None;
}
is_node_module_without_protocol(&module_name_trimmed).then_some(module_name)
}

fn diagnostic(_: &RuleContext<Self>, module_name: &Self::State) -> Option<RuleDiagnostic> {
Expand Down
28 changes: 22 additions & 6 deletions crates/biome_js_analyze/src/services/manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,24 +9,40 @@ use std::sync::Arc;

#[derive(Debug, Clone)]
pub struct ManifestServices {
pub(crate) manifest: Arc<PackageJson>,
pub(crate) manifest: Arc<Option<PackageJson>>,
}

impl ManifestServices {
pub(crate) fn is_dependency(&self, specifier: &str) -> bool {
self.manifest.dependencies.contains(specifier)
self.manifest
.as_ref()
.as_ref()
.map(|pkg| pkg.dependencies.contains(specifier))
.unwrap_or_default()
}

pub(crate) fn is_dev_dependency(&self, specifier: &str) -> bool {
self.manifest.dev_dependencies.contains(specifier)
self.manifest
.as_ref()
.as_ref()
.map(|pkg| pkg.dev_dependencies.contains(specifier))
.unwrap_or_default()
}

pub(crate) fn is_peer_dependency(&self, specifier: &str) -> bool {
self.manifest.peer_dependencies.contains(specifier)
self.manifest
.as_ref()
.as_ref()
.map(|pkg| pkg.peer_dependencies.contains(specifier))
.unwrap_or_default()
}

pub(crate) fn is_optional_dependency(&self, specifier: &str) -> bool {
self.manifest.optional_dependencies.contains(specifier)
self.manifest
.as_ref()
.as_ref()
.map(|pkg| pkg.optional_dependencies.contains(specifier))
.unwrap_or_default()
}
}

Expand All @@ -35,7 +51,7 @@ impl FromServices for ManifestServices {
rule_key: &RuleKey,
services: &ServiceBag,
) -> biome_diagnostics::Result<Self, MissingServicesDiagnostic> {
let manifest: &Arc<PackageJson> = services.get_service().ok_or_else(|| {
let manifest: &Arc<Option<PackageJson>> = services.get_service().ok_or_else(|| {
MissingServicesDiagnostic::new(rule_key.rule_name(), &["PackageJson"])
})?;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1595,5 +1595,3 @@ invalid.js:65:26 lint/style/useNodejsImportProtocol FIXABLE ━━━━━━


```


Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
import Buffer from "buffer";
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
---
source: crates/biome_js_analyze/tests/spec_tests.rs
expression: validDep.js
---
# Input
```jsx
import Buffer from "buffer";

```
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"dependencies": {
"buffer": "latest"
}
}
2 changes: 1 addition & 1 deletion crates/biome_project/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use biome_diagnostics::serde::Diagnostic;
use biome_parser::diagnostic::ParseDiagnostic;
use biome_rowan::Language;
pub use license::generated::*;
pub use node_js_project::{NodeJsProject, PackageJson};
pub use node_js_project::{Dependencies, NodeJsProject, PackageJson};
use std::any::TypeId;
use std::fmt::Debug;
use std::path::Path;
Expand Down
2 changes: 1 addition & 1 deletion crates/biome_project/src/node_js_project/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
mod package_json;

pub use crate::node_js_project::package_json::PackageJson;
pub use crate::node_js_project::package_json::{Dependencies, PackageJson};
use crate::{Manifest, Project, ProjectAnalyzeDiagnostic, ProjectAnalyzeResult, LICENSE_LIST};
use biome_json_syntax::JsonRoot;
use biome_rowan::Language;
Expand Down
16 changes: 16 additions & 0 deletions crates/biome_project/src/node_js_project/package_json.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ impl Dependencies {
pub fn contains(&self, specifier: &str) -> bool {
self.0.contains_key(specifier)
}

pub fn add(&mut self, dependency: impl Into<String>, version: impl Into<Version>) {
self.0.insert(dependency.into(), version.into());
}
}

#[derive(Debug, Clone)]
Expand All @@ -47,6 +51,18 @@ pub enum Version {
Literal(String),
}

impl From<&str> for Version {
fn from(value: &str) -> Self {
Self::Literal(value.to_string())
}
}

impl From<String> for Version {
fn from(value: String) -> Self {
Self::Literal(value)
}
}

impl Deserializable for PackageJson {
fn deserialize(
value: &impl DeserializableValue,
Expand Down