Support custom dependency nodes, such as exports

[gridsane]

First of all, I want to thank you for such a great plugin!

In our product we use FSD methology and it is crucial to us to keep track of boundaries violations.

But we discovered that boundaries can be broken not only by import statements. There are several other ways of introducing dependencies between modules:

• Re-exports. This problem is addressed in the Feature request: element-types should also check export #213
• Dynamic imports: const Module = import('path/to/module');
• Module mocks, for example: jest.mock

It would be very useful to allow users define their own dependency nodes.

I came up with the following solution:

1. Introduce the new setting boundaries/additional-dependency-nodes:

{
    // Format: [{ selector: '{esquery selector}', kind: '{value|type}' }]
    'boundaries/additional-dependency-nodes': [
        // Dynamic imports: import('source')
        { selector: 'ImportExpression > Literal', kind: 'value' },
        
        // Named exports: export { ModuleA } from 'modules/a'
        // Note ':not' pseudo-class here; it addresses parsers that do not have the exportKind property.
        { selector: 'ExportNamedDeclaration:not([exportKind=type]) > Literal', kind: 'value' },
        
        // Named type exports: export type { ModuleA } from 'modules/a'
        { selector: 'ExportNamedDeclaration[exportKind=type] > Literal', kind: 'type' },
        
        // And so on. We were able to describe all the cases we needed with this syntax.
    ]
}

2. Directly select Literal nodes to extract dependency source. This allows us to get rid of dependencyLocation helper and also simplifies how dependecy nodes are defined: we only need a single selector.

What do you think? Am I missing something?

If the solution is OK, I will finalize it by adding tests, validating the settings and updating the documentation.

closes #213

[javierbrea]

Hi @gridsane , I think that the solution is brilliant! It would be a great improvement for the plugin, thank you very much.
As you said, it would be needed to add documentation about it, tests, etc. Please let me know if I can help you somehow 😃

[gridsane]

Hi, @javierbrea! Could you please review the PR?

Changes:

• Added predefined settings for exports and dynamic imports.
• Added minimal tests to check the new setting. I used only two rules to check the settings: element-types to assert the functionality and external to verify export specifiers. I think copying settings for other rules would be excessive. What do you think?
• Updated the documentation and changelog.
• Also, added a guide entry to address incompatible changes (see below).

Questions:

1. Do we need to change error messages and rule descriptions, to avoid using the term "import"? For example, we could change "Importing unknown elements is not allowed" to "Using unknown elements is not allowed".
2. Do we need to rename the importKind option to simply kind? Now, it can be used in the context of export statements.

Backward compatibility issue and major version bump

I think it should be the new major version, due to incompatible changes in error position calculation. The issue is how dependencyLocation works with multiline imports:

// Single-line
import { ComponentB } from '../component-b/ComponentB.js'
// ------------------------^(start)---------------------^(end)
// { start: { line: 1, column: 27 }, end: { line: 1, column: 57 }}

// Multiline (here is the issue)
import {
// -------------------------^(start)
    ComponentB
} from '../component-b/ComponentB.js'
// --------------------------------------------------------^(end)
// { start: { line: 1, column: 29 }, end: { line:3, column: 59 }}

Referring to Literal nodes will fix the multiline behavior:

import {
    ComponentB
} from '../component-b/ComponentB.js'
// ----^(start)---------------------^(end)

So, if the eslint-disable-next-line directive was used to ignore a multiline import error, it will break in the next version.

[javierbrea]

praise: First of all, I'd like to congratulate you for such PR in a repository that, honestly, has a lack of development documentation and needs a refactor urgently for improving the development experience. I imagine it hasn't been easy for you, specially to implement unit tests, given its current complexity. So, thank you very much! It deserves also special mention how you followed the current code style and guidelines, specially when they are not documented! 😃

Said that, I think that the change is important enough to give it more relevance in the docs. We should mention in the introduction that the plugin now is not only able to check imports, but also any other type of dependency. There are also other details that we should mention in the configuration of rules, such as those related to importKind or specifiers, because, obviously, they will work only when the dependency nodes support them.

So, if you don't mind, I can make a little review of the docs after merging the PR into the release branch for mentioning these details. My intention is to publish this as soon as possible, so I will make the minimum changes necessary for the moment.

Once this version is published, I will probably create issues for enabling the "export" and "dynamic-import" dependency nodes by default in an oncoming major version, and to review the whole documentation for making it clearer, given that the configuration is becoming more powerful and complex every new release.

[gridsane]

I completely agree that the best way is to carefully update the documentation. I'll leave the rest of the documentation untouched, so you can make proper changes.

I like your idea of making 'export' and 'dynamic-import' the defaults in the major release.
However, I just realized that the setting 'additional-dependency-nodes' doesn't permit changes to default values.

Could we rename it to just "dependency-nodes" and allow users to redefine it?

In this case, it would be possible to maintain backward compatibility by redefining it to ['import']. In this scenario, we could leave constants as they are. Duplicated entries will not affect the performance because the final result is an object with selectors as keys. Duplicates will be eliminated, and ESLint will not traverse the same node twice.

[javierbrea]

@gridsane, you're right about duplicated entries, I didn't notice that the final result is an object, so there is no a performance problem, I'll resolve that conversation.

It's also a good idea to provide users a way to deactivate default dependency nodes, but I don't like too much that users that may want to define their own dependency nodes would have to redefine again the default ones. What about having two different settings, one allowing to redefine the dependency-nodes, and another one enabling to add additional ones? It would look like:

{
    // Next setting allows to enable/disable built-in default dependency nodes.
    // In the first version, "export" and "dynamic-import" should be supported,
    // but they should be disabled by default for an easier version adoption.
    // In an oncoming major version they will be enabled by default,
    // but users will be able to deactivate them by using this setting.
    "boundaries/dependency-nodes": ["import", "export", "dynamic-import"],

   // Next setting allows to add custom dependency-nodes.
   // It does not affect to the default ones, so, users don't have to redefine
   // defaults in case they want all defaults enabled and they only want to add more.
    "boundaries/additional-dependency-nodes": [
        {
           selector: "CallExpression[callee.object.name=jest][callee.property.name=mock] > Literal:first-child",
           kind: "value"
        }
    ]
}

The boundaries/dependency-nodes setting should support only strings, and only those that are defined as built-in dependency nodes in the plugin, and the boundaries/additional-dependency-nodes should support only objects.

[gridsane]

I reorganized the settings as you suggested. I'm not sure about the changes in "CHANGELOG.md". Should I delete or modify the "Breaking changes" section and the guide?

If there is anything I can tweak, just let me know.

[javierbrea]

I reorganized the settings as you suggested. I'm not sure about the changes in "CHANGELOG.md". Should I delete or modify the "Breaking changes" section and the guide?

Don't worry @gridsane , I'll review the docs in the release branch.