Skip to content

chore: enable @typescript-eslint/ban-ts-comment#11326

Merged
patak-cat merged 2 commits intovitejs:mainfrom
sapphi-red:chore/enable-ban-ts-comment
Dec 12, 2022
Merged

chore: enable @typescript-eslint/ban-ts-comment#11326
patak-cat merged 2 commits intovitejs:mainfrom
sapphi-red:chore/enable-ban-ts-comment

Conversation

@sapphi-red
Copy link
Member

Description

superseds close #8413

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@sapphi-red sapphi-red added the p1-chore Doesn't change code behavior (priority) label Dec 12, 2022
path.get('body').forEach((p) => {
if (t.isImportDeclaration(p)) {
// @ts-expect-error
if (t.isImportDeclaration(p.node)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason for this change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

t.isImportDeclaration only expects Node to be passed to the argument. p is NodePath and p.node is Node.
The previous code also works because NodePath and Node both has type property.
https://github.com/babel/babel/blob/df733b18ae88f370caddecc30c6d96844007c411/packages/babel-types/src/validators/generated/index.ts#L1062-L1078
But given that t.isImportDeclaration narrows the argument to ImportDeclaration instead of NodePath<ImportDeclaration>, passing p.node is more correct.

We could use p.isImportDeclaration() instead of t.isImportDeclaration(p.node), too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation, it looks good as is!

@Shinigami92 Shinigami92 dismissed their stale review December 12, 2022 10:53

the default option already contains what I requested

@patak-cat patak-cat merged commit e58a4f0 into vitejs:main Dec 12, 2022
@sapphi-red sapphi-red deleted the chore/enable-ban-ts-comment branch December 12, 2022 11:09
futurGH pushed a commit to futurGH/vite that referenced this pull request Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

p1-chore Doesn't change code behavior (priority)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants