| CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
feat: support TS syntax in no-restricted-imports
#19562
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
Conversation
✅ Deploy Preview for docs-eslint canceled.
|
snitin315
force-pushed
the
ts/no-restricted-imports
branch
from
d4d142d to
dffbc64
Compare
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.
There seems to be either extra functionality or missing tests with allowedTypeImportPathNameSet / isAllowedTypeImportPath.
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 we can actually get rid of a bunch of this extension code! 🔪
|
@JoshuaKGoldberg This needs a re-review. |
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 core functionality looks perfect to me. Just requesting changes on docs (sorry for not till now) and a bit more tests. 🚀
snitin315
added
the
accepted
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.
Can you also update the types for this rule in lib/types/rules.d.ts?
|
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
|
@JoshuaKGoldberg @mdjermanovic I have addressed your feedback. |
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 great, nicely done! 👏
|
Hi everyone, it looks like we lost track of this pull request. Please review and see what the next steps are. This pull request will auto-close in 7 days without an update. |
|
@snitin315 can you check the CI failures? |
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
lib/rules/no-restricted-imports.js
Outdated
| if (allowTypeImports) { | ||
| return; | ||
| } |
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.
| if (allowTypeImports) { | |
| return; | |
| } | |
| if ( | |
| allowTypeImports && | |
| isTypeOnlySpecifier(specifier.specifier) | |
| ) { | |
| return; | |
| } |
Looks like the isTypeOnlySpecifier() check has been removed instead of fixed (#19562 (comment)).
For example, this should not pass:
/* eslint no-restricted-imports: ["error", { paths: [{
name: "foo",
allowImportNames: ["bar"],
allowTypeImports: true
}]}] */
import { baz } from "foo";
snitin315
force-pushed
the
ts/no-restricted-imports
branch
3 times, most recently
from
ae521d9 to
94d8d83
Compare
|
Can you fix lint 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.
LGTM, thanks! Leaving open for a second review.
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.
🔥 wonderful!
@mdjermanovic's thorough review makes me worried about edge cases existing in the typescript-eslint extension rule. I am relieved to see this PR land. 😄
github-project-automation
bot
moved this from Second Review Needed
to Complete
in Triage
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)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
Refers #19173
It adds support for type import syntaxes:
import type X from "..."import { type X } from "..."import x = require("...")Is there anything you'd like reviewers to focus on?
https://typescript-eslint.io/rules/no-restricted-imports/