Closed gridsane closed 7 months ago
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 😃
Hi, @javierbrea! Could you please review the PR?
Changes:
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?Questions:
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.
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.
@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.
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.
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.
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:
const Module = import('path/to/module');
It would be very useful to allow users define their own dependency nodes.
I came up with the following solution:
boundaries/additional-dependency-nodes
:Literal
nodes to extract dependency source. This allows us to get rid ofdependencyLocation
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