New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat!: no-restricted-imports allow multiple config entries for same path #18021
Conversation
✅ Deploy Preview for docs-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
{ | ||
code: "import * as mod from 'mod'", | ||
options: [{ | ||
paths: [ | ||
{ | ||
name: "mod", | ||
importNames: ["foo"], | ||
message: "Import foo from qux instead." | ||
}, | ||
{ | ||
name: "mod", | ||
importNames: ["bar"], | ||
message: "Import bar from qux instead." | ||
} | ||
] | ||
}], | ||
errors: [ | ||
{ | ||
message: "* import is invalid because 'foo' from 'mod' is restricted. Import foo from qux instead.", | ||
type: "ImportDeclaration", | ||
line: 1, | ||
column: 8, | ||
endColumn: 16 | ||
}, | ||
{ | ||
message: "* import is invalid because 'bar' from 'mod' is restricted. Import bar from qux instead.", | ||
type: "ImportDeclaration", | ||
line: 1, | ||
column: 8, | ||
endColumn: 16 | ||
} | ||
] | ||
}, |
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.
Perhaps this could be a single error with a concatenated message, but that would be complicated as these texts are generated by interpolating data
, and I'm not sure if one big message would be better than multiple errors.
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 think it's fine the way it is. If people are configuring the rule this way then we can let them decide to change how it's configured if this isn't the behavior they want.
{ | ||
code: "import { bar } from 'mod'", | ||
options: [{ | ||
paths: [ | ||
|
||
// restricts importing anything from the module | ||
{ | ||
name: "mod" | ||
}, | ||
|
||
// message for a specific import name | ||
{ | ||
name: "mod", | ||
importNames: ["bar"], | ||
message: "Import bar from qux instead." | ||
} | ||
] | ||
}], | ||
errors: [ | ||
{ | ||
message: "'mod' import is restricted from being used.", | ||
type: "ImportDeclaration", | ||
line: 1, | ||
column: 1, | ||
endColumn: 26 | ||
}, | ||
{ | ||
message: "'bar' import from 'mod' is restricted. Import bar from qux instead.", | ||
type: "ImportDeclaration", | ||
line: 1, | ||
column: 10, | ||
endColumn: 13 | ||
} | ||
] | ||
}, |
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 produces two errors: one for the whole import, and another specifically for the imported name bar
. I think it's fine and technically correct.
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.
Agreed.
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.
Overall LGTM. Would it make sense to add some tests where importNames
has more than one element?
I've now added a couple of tests where |
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.
LGTM. Thanks!
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[x] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
Fixes #15261
What changes did you make? (Give an overview)
Updated
no-restricted-imports
to check all config entries that have a matchingname
instead of checking just the last one.Is there anything you'd like reviewers to focus on?
Matching entries produce errors separately from each other. I believe this is the simplest yet the most flexible solution, but a downside may be that in some cases the rule produces multiple errors (see the added test cases).
TODO: update migration guide