-
Notifications
You must be signed in to change notification settings - Fork 664
feat(rome_js_analyze): new lint rule useImportRestrictions
#4638
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,172 @@ | ||||||||||
use bpaf::Bpaf; | ||||||||||
use rome_analyze::{context::RuleContext, declare_rule, Ast, Rule, RuleDiagnostic}; | ||||||||||
use rome_console::markup; | ||||||||||
use rome_deserialize::{ | ||||||||||
json::{has_only_known_keys, VisitJsonNode}, | ||||||||||
DeserializationDiagnostic, VisitNode, | ||||||||||
}; | ||||||||||
use rome_js_syntax::JsModuleSource; | ||||||||||
use rome_json_syntax::{JsonLanguage, JsonSyntaxNode}; | ||||||||||
use rome_rowan::{AstNode, SyntaxTokenText}; | ||||||||||
use serde::{Deserialize, Serialize}; | ||||||||||
use std::{path::Path, str::FromStr}; | ||||||||||
|
||||||||||
#[cfg(feature = "schemars")] | ||||||||||
use schemars::JsonSchema; | ||||||||||
|
||||||||||
declare_rule! { | ||||||||||
/// Forbids importing from nested modules. | ||||||||||
/// | ||||||||||
/// For larger code bases, it can be undesirable to let any file arbitrarily import any other | ||||||||||
/// files. Arbitrary imports can lead to cycles that may be hard to debug, so it may be | ||||||||||
/// advisable to specify which files may import from which other files. | ||||||||||
/// | ||||||||||
/// A useful rule of thumb is that for modules that consist of several files, only the module's | ||||||||||
/// `index.js` or `index.ts` may be imported directly from outside that module, while symbols | ||||||||||
/// from other files should only be considered "public" if they're re-exported from the index. | ||||||||||
/// | ||||||||||
/// This rule treats nested imports as an attempt to access "private" internals of a module. | ||||||||||
/// Only exports defined by the `index.js` or `index.ts` are allowed to be imported externally. | ||||||||||
/// Effectively, this means that you may not directly import any files or subdirectories that | ||||||||||
/// are not siblings to the file you're in, or any of its ancestors. | ||||||||||
/// | ||||||||||
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. The description is about nested modules. What about parent and ancestors modules? |
||||||||||
/// This rule only applies to relative imports, since the API from external dependencies is | ||||||||||
/// often out of your control. | ||||||||||
/// | ||||||||||
/// ## Examples | ||||||||||
/// | ||||||||||
/// ### Invalid | ||||||||||
/// | ||||||||||
/// ```js,expect_diagnostic | ||||||||||
/// import { privateInternals } from "../aunt/cousin"; | ||||||||||
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. I could add the file extension to clarify what is being imported. This is required in ESM.
Suggested change
It might help to add an explanation of why the example was rejected. We could also add more examples: import { internal } from "./sibling/nephew.js"; import { internal } from "../../index.js"; |
||||||||||
/// ``` | ||||||||||
/// | ||||||||||
/// ### Valid | ||||||||||
/// | ||||||||||
/// ```js | ||||||||||
/// import { publicExport } from "./sibling"; | ||||||||||
/// import { reexportedInternals } from "../aunt"; | ||||||||||
Comment on lines
+47
to
+48
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. I could add the file extension to clarify the examples (I was wondering: is it
Suggested change
We could also separate the two examples and add a comment to explain why each one is valid. We should also add the following example? import { publicExport } from "./sibling/index.js"; |
||||||||||
/// ``` | ||||||||||
/// | ||||||||||
pub(crate) NoNestedModuleImports { | ||||||||||
version: "next", | ||||||||||
name: "noNestedModuleImports", | ||||||||||
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. Did we decide on the rule name? |
||||||||||
recommended: false, | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
impl Rule for NoNestedModuleImports { | ||||||||||
type Query = Ast<JsModuleSource>; | ||||||||||
type State = (); | ||||||||||
type Signals = Vec<Self::State>; | ||||||||||
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. It would be best if you used In this case, we signal with Doing so, will simply the code: // just this
is_public_import(path, ctx.options()) 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. I agree with the previous comment. |
||||||||||
type Options = ImportOptions; | ||||||||||
|
||||||||||
fn run(ctx: &RuleContext<Self>) -> Self::Signals { | ||||||||||
let binding = ctx.query(); | ||||||||||
let Ok(path) = binding.inner_string_text() else { | ||||||||||
return Vec::new(); | ||||||||||
}; | ||||||||||
|
||||||||||
if is_public_import(path, ctx.options()) { | ||||||||||
Vec::new() | ||||||||||
} else { | ||||||||||
vec![()] | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
fn diagnostic(ctx: &RuleContext<Self>, _: &Self::State) -> Option<RuleDiagnostic> { | ||||||||||
ctx.query().inner_string_text().ok().map(|path| { | ||||||||||
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. We already query |
||||||||||
let path = path.text(); | ||||||||||
let parent = Path::new(path) | ||||||||||
.parent() | ||||||||||
.and_then(Path::to_str) | ||||||||||
.unwrap_or_default(); | ||||||||||
|
||||||||||
RuleDiagnostic::new( | ||||||||||
rule_category!(), | ||||||||||
ctx.query().range(), | ||||||||||
markup! { | ||||||||||
"Importing from nested modules is not allowed." | ||||||||||
}, | ||||||||||
) | ||||||||||
.note(markup! { | ||||||||||
"Please import from "<Emphasis>{parent}</Emphasis>" instead " | ||||||||||
"(you may need to re-export from "<Emphasis>{path}</Emphasis>")." | ||||||||||
}) | ||||||||||
}) | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
fn is_public_import(module_path: SyntaxTokenText, options: &ImportOptions) -> bool { | ||||||||||
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. We could explain what a "public import" is 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.
|
||||||||||
if !module_path.starts_with('.') { | ||||||||||
return true; | ||||||||||
} | ||||||||||
|
||||||||||
// Make an exception for loading resoures directly: | ||||||||||
for allowed_extension in &options.allowed_extensions { | ||||||||||
if module_path.ends_with(allowed_extension) { | ||||||||||
return true; | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
module_path | ||||||||||
.split('/') | ||||||||||
.filter(|&part| part != "." && part != "..") | ||||||||||
.count() | ||||||||||
<= 1 | ||||||||||
} | ||||||||||
|
||||||||||
/// Options for the rule `noNestedModuleImports`. | ||||||||||
#[derive(Default, Deserialize, Serialize, Debug, Clone, Bpaf)] | ||||||||||
#[cfg_attr(feature = "schemars", derive(JsonSchema))] | ||||||||||
#[serde(rename_all = "camelCase", deny_unknown_fields)] | ||||||||||
pub struct ImportOptions { | ||||||||||
/// List of extensions that are always allowed to be imported. | ||||||||||
pub allowed_extensions: Vec<String>, | ||||||||||
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. I really don't like this configuration. I think we should apply the restriction on extension-less imports (to support TypeScript old-fashion imports) and files endings with |
||||||||||
} | ||||||||||
|
||||||||||
impl FromStr for ImportOptions { | ||||||||||
type Err = (); | ||||||||||
|
||||||||||
fn from_str(_s: &str) -> Result<Self, Self::Err> { | ||||||||||
Ok(Self::default()) | ||||||||||
} | ||||||||||
} | ||||||||||
|
||||||||||
impl VisitJsonNode for ImportOptions {} | ||||||||||
impl VisitNode<JsonLanguage> for ImportOptions { | ||||||||||
fn visit_member_name( | ||||||||||
&mut self, | ||||||||||
node: &JsonSyntaxNode, | ||||||||||
diagnostics: &mut Vec<DeserializationDiagnostic>, | ||||||||||
) -> Option<()> { | ||||||||||
has_only_known_keys(node, &["allowedExtensions"], diagnostics) | ||||||||||
} | ||||||||||
|
||||||||||
fn visit_map( | ||||||||||
&mut self, | ||||||||||
key: &JsonSyntaxNode, | ||||||||||
value: &JsonSyntaxNode, | ||||||||||
diagnostics: &mut Vec<DeserializationDiagnostic>, | ||||||||||
) -> Option<()> { | ||||||||||
let (name, value) = self.get_key_and_value(key, value, diagnostics)?; | ||||||||||
let name_text = name.text(); | ||||||||||
if name_text == "allowedExtensions" { | ||||||||||
let array = value.as_json_array_value()?; | ||||||||||
|
||||||||||
for element in array.elements() { | ||||||||||
let element = element.ok()?; | ||||||||||
|
||||||||||
if let Some(extension) = element.as_json_string_value() { | ||||||||||
self.allowed_extensions.push(extension.to_string()); | ||||||||||
} else { | ||||||||||
diagnostics.push(DeserializationDiagnostic::new(markup! { | ||||||||||
"The field "<Emphasis>"allowedExtensions"</Emphasis>" must contain an array of strings" | ||||||||||
}) | ||||||||||
.with_range(element.range())); | ||||||||||
} | ||||||||||
} | ||||||||||
} | ||||||||||
Some(()) | ||||||||||
} | ||||||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
|
||
import "../aunt/cousin"; | ||
import { someSymbol } from "./module/private"; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
--- | ||
source: crates/rome_js_analyze/tests/spec_tests.rs | ||
expression: invalid.js | ||
--- | ||
# Input | ||
```js | ||
|
||
import "../aunt/cousin"; | ||
import { someSymbol } from "./module/private"; | ||
|
||
``` | ||
|
||
# Diagnostics | ||
``` | ||
invalid.js:2:8 lint/nursery/noNestedModuleImports ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ | ||
|
||
! Importing from nested modules is not allowed. | ||
|
||
> 2 │ import "../aunt/cousin"; | ||
│ ^^^^^^^^^^^^^^^^ | ||
3 │ import { someSymbol } from "./module/private"; | ||
4 │ | ||
|
||
i Please import from ../aunt instead (you may need to re-export from ../aunt/cousin). | ||
|
||
|
||
``` | ||
|
||
``` | ||
invalid.js:3:28 lint/nursery/noNestedModuleImports ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ | ||
|
||
! Importing from nested modules is not allowed. | ||
|
||
2 │ import "../aunt/cousin"; | ||
> 3 │ import { someSymbol } from "./module/private"; | ||
│ ^^^^^^^^^^^^^^^^^^ | ||
4 │ | ||
|
||
i Please import from ./module instead (you may need to re-export from ./module/private). | ||
|
||
|
||
``` | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
/* should not generate diagnostics */ | ||
|
||
import "./sibling"; | ||
import { someSymbol } from "../aunt"; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
--- | ||
source: crates/rome_js_analyze/tests/spec_tests.rs | ||
assertion_line: 103 | ||
expression: valid.js | ||
--- | ||
# Input | ||
```js | ||
/* should not generate diagnostics */ | ||
|
||
import "./sibling"; | ||
import { someSymbol } from "../aunt"; | ||
|
||
``` | ||
|
||
|
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.
I removed this block from the
CONTRIBUTING
guide sinceDeserializableRuleOptions
is nowhere to be found anymore, so I assume it's no longer relevant.