☂️ Create useImportRestrictions rule

[arendjr]

Description

Based on the original discussion in the PR for the noNestedModuleImports rule, I will start working on a new rule called useImportRestrictions. This will be done in several steps:

• Create MVP that closely mirrors the original PR, but with a few changes:
  • It will disallow importing from parent index files, based on @Conaclos suggestion here: feat(rome_js_analyze): new lint rule useImportRestrictions #4638 (review) At the moment, I don't foresee the need to put this behind a config option. Update: Skipping on this, since it seems redundant with cycle detection.
  • There will not be an allowedExtensions option anymore. Instead, it will only operate on standard JS/TS extensions and ignore all others. See the list of extensions: feat(rome_js_analyze): new lint rule useImportRestrictions #4638 (comment)
  • Note: The initial version may perform repetitive fs calls to check for the existence of index files. I think this should be acceptable for this initial version given that Rome has no accepted solution for import resolutions yet. Update: No longer necessary since I've moved requireIndexFile to a separate improvement.
• A requireIndexFile?: boolean config option can be added, which effectively represents a "strict mode". If false (the default), the rule will only fail when directories have an index file, but the user imports from a sibling of that index file. If true, directories must have an index file in order to allow anything to be imported from them at all.
  • Before this option is implemented, the rule will always behave as if it is set to true, since we can't detect the presence of index files without file system access. This also means implementing it efficiently will require support for workspace-level file system access in Rome.
• A packagePrivate option will be added with the possible values "all", "annotated", and "none". The default value will be "none", which will be a breaking change, since the initial version will act like the value "all". The interpretations are as follows:
  • "all": All exported symbols are treated as if they're package private, meaning they cannot be exported directly from a path adjacent to an index file. To be imported from outside their "package", they will need to be re-exported by the index file. See also: https://github.com/uhyo/eslint-plugin-import-access
  • "annotated": Only treats symbols as package private if they are annotated as @package or @access package inside JSDoc comments. See also: https://jsdoc.app/tags-package.html
  • "none": No symbols will be treated as package private, meaning this part of the rule's restrictions are effectively disabled.
• The strictDependencies option will be added similar to eslint-plugin-strict-dependencies. The exact implementation is TBD.
• A new config option allowCycles will be added, which will forbid the presence of import cycles if its value is false. I'd suggest false to also be the new default from that moment on. See also: lint rule: no-cycle #4187

Other suggestions (such as options from eslint-plugin-import) are welcome to be discussed here :)

[Conaclos]

Thanks for opening the discussion about the rule :)

I wonder if we could get rid of the packagePrivate option altogether.

If there is an index file, then all exports of sibling modules (i.e. modules in the same directory) are package-scoped.
Otherwise, they are all public.

Regardless of the presence or absence of an index file, any module that has at least one export annotated with @package or @access package has the following behavior: any annotated exports are package-scoped; other exports are public.

By the way, if the rule checks for cycles, I no longer see the value of forbidding imports from parent indexes?
Do we really need a config to disable cycle checks?

I also wonder if the rule should not be divided into three rules: one focused on package-scoped exports (index file and annotated exports) ; another one to forbid cycles ; and a last one for strict dependencies ? Could you explain your rationale behind an all-inclusive rule?

[ematipico]

Note: The initial version may perform repetitive fs calls to check for the existence of index files. I think this should be acceptable for this initial version given that Rome has no accepted solution for import resolutions yet.

I highly suggest to avoid this, for the following reasons:

• we can't test it
• it's a rabbit hole: symbol links, aliases, nodejs resolutions, etc.
• real URLs
• protocols (http://, file://, etc)
• it doesn't seem to be a place where a rule should check if a file exists

We should just assume that the code is there and it works. The rules from the plugin import can wait

[arendjr]

@ematipico wrote:

I highly suggest to avoid this

Fair enough :) It does mean I wouldn’t be able to implement the requireIndexFile option initially, but I can postpone that until a better import resolution mechanism is available. It does also mean @Conaclos’s suggestion to use the presence of an index file to determine whether siblings are exported publicly or not would not work yet either, but the packagePrivate option as I had it in mind would have needed to wait until we have project-level metadata anyway, so that’s also not an immediate concern.

@Conaclos also wrote:

By the way, if the rule checks for cycles, I no longer see the value of forbidding imports from parent indexes?

Good point. If we forbid parent indices initially, we can probably remove it again at that point. Which makes me think we might as well save ourselves the effort :)

Do we really need a config to disable cycle checks?

I felt it might be a good idea to allow disabling. Technically they’re allowed by ESM, and they only lead to problems in some cases. At the same time, I wouldn’t disable such check myself, and we could also opt to not offer it as an option until someone makes a convincing case for it.

I also wonder if the rule should not be divided into three rules: one focused on package-scoped exports (index file and annotated exports) ; another one to forbid cycles ; and a last one for strict dependencies ? Could you explain your rationale behind an all-inclusive rule?

The main reason is because I felt they’re all highly related and operate on the same syntax nodes. But as you point out, there’s also room for splitting if you feel that makes more sense.

Maybe having it as one also makes the options more easily discoverable? Personally if I find a valuable rule I would intuitively read the list of options once to get an idea of what it offers, but I wouldn’t have a complete overview of all the rules Rome offers (on a barely related note, I would like if I could enable Rome’s React rules with a single toggle instead of enabling individual rules).

[arendjr]

Thinking about the implementation a bit more, I think my preference grows towards a single rule. After all, it doesn’t matter if an import that violates the strictDependencies also introduces a cycle, or if a cycle-introducing import happens to do so through a package private symbol. But if the rules are split, users will get multiple errors, even if it’s likely it only takes a single action to correct.

Performance-wise, especially when things like import resolutions come into play, it’s probably better if only a single rule has to perform import resolution rather than multiple ones. We probably may want a cache anyway to prevent repetitive resolutions, but such a cache would be easier to implement if it doesn’t need to be shared across rules.

[Conaclos]

Without module resolution, the rule seems impossible to implement.
In this case, I have another idea: the rule could focus on strictDependencies
For example, a user could restrict imports to the modules of the same directory and make indexes public with the following configuration:

{
 "./**/*.js": ["$targetDirectory/*.js"],
 "./**/index.js": ["./**/*.js"],
}

$targetDirectory corresponds to the directory where the imported module is.
I don't like the $targetDirectory. However, this allows to express the idea.

[arendjr]

Well, the original PR I made implemented an MVP and didn't rely on module resolution, so that's what I intend to focus on again for the first version. It doesn't even require any configuration if we postpone the requireIndexFile option.