-
Notifications
You must be signed in to change notification settings - Fork 664
feat(rome_js_analyze): new lint rule useImportRestrictions
#4638
Conversation
✅ Deploy Preview for docs-rometools canceled.Built without sensitive environment variables
|
} | ||
} | ||
``` | ||
|
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 since DeserializableRuleOptions
is nowhere to be found anymore, so I assume it's no longer relevant.
This does not seem to be a widespread discipline. Also, it might conflict with the practice of sub-communities (deno comes to mind). Some suggestions to move forward:
|
noNestedModuleImport
I agree it’s not a very widespread practice, AFAIK, though it kinda matches what Rust does where modules have to explicitly mark their submodules as public to make them importable from external modules. JS/TS doesn’t offer something like that at the language level, but I think this rule fills a gap there. A possible solution to make it compatible with the Deno ecosystem would be to allow configuring module index file names, so that The configuration is just to allow exceptions for things like resource files, such as images or CSS files that may need to be imported from otherwise illegal paths. But the way it’s currently implemented isn’t sufficient to make a clean exception for Thinking about this a little bit more, I know there are also quite some projects that have strict requirements regarding how libraries such as Lodash are imported (either mandating the use of specific paths per function, or forbidding such usage). I think it’s not unreasonable to generalize this rule, so that such practices could also be covered by it, although it would probably make the configuration a bit more complex. Just let me know if you see a reasonable chance this could be accepted into Rome, and if so, how you would like to see this evolve :) |
We could apply the rule only to path ending with a specific extensions (js, cjs, mjs, ts, cts, mts, jsx, tsx, .d.ts) and potentially files without extension.
To be honest I am not a big fan of the rule. This looks too specific and restrictive. I could accept the rule if the rule could only concern folders containing a |
noNestedModuleImport
noNestedModuleImport
I start by saying that this is a rule we could have in our arsenal. I'm in favour ⏫ I don't like it, but that doesn't mean that we should deny it. The fact that it's used in a work environment surely is a positive signal. Regarding the implementation, I don't think we would a configuration, here's why:
Also, the rule should make sure that the |
I am relatively in favor of this rule. I believe that many JS/TS users are looking for lint rules regarding the scope of package release. FYI: Another way to achieve a similar goal to this rule is to use JSDoc annotations as follows: (However, we would probably need to parse JSDoc to achieve something similar to this |
Thank you for your contributions! I like some kinds of this rule. These tools includes @unvalley's mentioned On the other hand, I also feel that this rule is specific and limited. Before adding this rule, I want to understand the use case of this rule in more detail. |
Thanks for all the feedback! So I think it's fair to say there's sufficient support to move this rule forward in some shape or form, which is great to hear :) The question remains however what exactly it should look like. I appreciate the links to the
I also like the suggestions for checking for the existence of an So assuming we want to generalize this rule to incorporate some of the features of the linked rules, I would suggest renaming this rule to {
/**
* Filenames that are recognized as index files.
*
* Default: `["index.js", "index.ts", "index.tsx"]`
*/
// FIXME: It feels like this setting should not be rule-specific? Does Rome already know somehow
// which files may be index files based on project configuration?
indexFilenames?: Array<string>,
/**
* If `true`, requires all directories to have an index file in order to allow importing from them.
*
* Default: `false`
*/
requireIndexFile?: boolean,
/**
* Which exports should be considered package private?
*
* Package private exports may only be imported from the same directory as the one in
* which they are declared. Package private exports may be re-exported from an index file
* to widen their scope.
*
* The "annotated" setting requires exported symbols to be annotated with an `@package` or
* `@access package` JSDoc comment to be considered package private.
*
* Default: `none`
*/
// TODO: I hope the initial version of this rule can be implemented without this option,
// in which case it would default to "all" until this is actually implemented.
packagePrivate?: "all" | "annotated" | "none",
/**
* Allows for strict definitions of which modules may import other modules.
*
* See: https://www.npmjs.com/package/eslint-plugin-strict-dependencies#usage
*/
// TODO: I hope the initial version of this rule can be implemented without this option,
// so we can flesh out the details of this later.
strictDependencies?: Array<ModuleDependencyDefinition>,
} As mentioned above, I hope we can merge an initial version of this rule without needing to implement all the configuration in a single PR. Please let me know your thoughts! One last note: I also saw in the
Seeing how Rome has a Language Server too, would it be possible to implement this behavior there too? Or does it really require a plugin, since it seems to be about hiding results coming from the main TS LS? It would be really awesome if this could "just work" with Rome... |
We don't have this functionality just yet. For now, we can ignore it. |
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 understood that it would take a few PRs to get the rule right. @arendjr, could you please create an issue to track the works needed for this rule to land?
I left a few comments, could you please look at them?
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 comment
The reason will be displayed to describe this comment to others. Learn more.
It would be best if you used Option
instead. You use Vec
when you need to return multiple violations.
In this case, we signal with None
when the run didn't detect anything, and Some(())
when the rule should trigger.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the previous comment.
} | ||
|
||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
We already query path
inside run
, wouldn't it be wiser to store it inside the State
of the rule? No need to repeat the code.
} | ||
} | ||
|
||
fn is_public_import(module_path: SyntaxTokenText, options: &ImportOptions) -> bool { |
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.
We could explain what a "public import" is
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.
is_nested_or_parent_import
?
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.
This file seems to be empty. We should remove it
.as_json_string_value() | ||
.and_then(|extension| extension.inner_string_text().ok()) | ||
{ | ||
self.allowed_extensions.push(extension.to_string()); |
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.
We have to be careful with the configuration. A user could add .js
to the list allowed extensions and bypass the rule completely
@ematipico My idea was to turn this PR into a PoC effectively by renaming the rule, making sure the current extension configuration is no longer needed and implementing the Then we could do 2 follow-ups for the I would still like some advice regarding the |
I am not sure we really need |
Actually, I think the list can be cut down to only include JS extensions. In JSX projects I don't think I have ever seen importing .jsx extensions. TypeScript projects don't use TS extensions in their imports, and since we are only targeting only relative imports, Deno should be fine. |
@arendjr sure, your plan sounds good to me :) feel free to drive the developments as you want. I'm happy to merge the PR - if the others are happy too - whenever you want. While a rule is under nursery, you can do all the changes that you want without worrying about semver. Creating an issue would allow us to keep the discussion away from the PR, without bloating it with too many comments |
Typescript |
noNestedModuleImport
useImportRestrictions
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.
Looks good to me. @Conaclos what do you think?
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 have some questions and suggestions.
If I understand the rule correctly, arbitrary imports of index files are authorized?
import { X } from "../../../index.js"
import { Y } from "./nested/nested/nested//index.js"
import { Z } from "./index.js"
Importing from a local or ancestor index seems very suspicious.
There is high probability of cycles.
Importing a local or ancestor file seems safer than importing a local or ancestor eindex.
// This has a high probability of cycles
import { X } from "../../../index.js"
// This looks safer?
import { X } from "../../../a.js"
In this case, should we reverse the check for local and ancestor modules. i.e. forbidding imports of index files and allowing imports of other files? Or should we forbid import of any files (including index files) in ancestor modules?
In fact, I realize that the rule could be even more strict and completely erase the possibility of cycles (except if we consider symlink, import maps, and package.json
's imports
filed): a file could be allowed to only import files in submodules. This could guarantee a directed acyclic graph of dependencies.
However, this looks very restrictive.
} | ||
} | ||
|
||
fn is_public_import(module_path: SyntaxTokenText, options: &ImportOptions) -> bool { |
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.
is_nested_or_parent_import
?
/// import { publicExport } from "./sibling"; | ||
/// import { reexportedInternals } 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 could add the file extension to clarify the examples (I was wondering: is it sibling.js
or sibiling/index.js
?. Moreover, file extensions are required in ESM.
/// import { publicExport } from "./sibling"; | |
/// import { reexportedInternals } from "../aunt"; | |
/// import { publicExport } from "./sibling.js"; | |
/// import { reexportedInternals } from "../aunt.js"; |
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";
/// ### Invalid | ||
/// | ||
/// ```js,expect_diagnostic | ||
/// import { privateInternals } from "../aunt/cousin"; |
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 could add the file extension to clarify what is being imported. This is required in ESM.
/// import { privateInternals } from "../aunt/cousin"; | |
/// import { privateInternals } from "../aunt/cousin.js"; |
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";
#[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 comment
The 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 {.js,.ts,.cjs,.cts,.mjs,.mts}
.
/// 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 comment
The reason will be displayed to describe this comment to others. Learn more.
The description is about nested modules. What about parent and ancestors modules?
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 comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with the previous comment.
/// | ||
pub(crate) NoNestedModuleImports { | ||
version: "next", | ||
name: "noNestedModuleImports", |
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.
Did we decide on the rule name?
@Conaclos Thanks for the review! I'll take your comments into account when I reopen the PR. For now I think it's better I close it to avoid confusion, sorry about that! My intention is to reopen it with the rule name All that said, initially I would like to continue working on this rule in "MVP form" by reopening it with the new name, but without the extension configuration. I think I received some good feedback on how to avoid that. I also like your point about disallowing the import of a parent index, which makes a lot of sense, so I think I'll include that in the next PR too. Hopefully it won't take me too long to reopen it :) |
Sorry for opening this PR without prior discussion. The "real" rule I want to implement is #4020, but I figured I'd first try my hand on something that looked a little simpler on the outset :) Anyway, it's a rule we use at work, and it would be nice if you were willing to accept it...
Summary
Per the rule's documentation:
Test Plan
Test cases for the rule are included.