Skip to content
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

Rules: Deprecate prefer-ts-expect-error in favor of ban-ts-comment #8333

Closed
2 tasks done
fregante opened this issue Feb 1, 2024 · 7 comments · Fixed by #9081
Closed
2 tasks done

Rules: Deprecate prefer-ts-expect-error in favor of ban-ts-comment #8333

fregante opened this issue Feb 1, 2024 · 7 comments · Fixed by #9081
Labels
accepting prs Go ahead, send a pull request that resolves this issue package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Milestone

Comments

@fregante
Copy link
Contributor

fregante commented Feb 1, 2024

Before You File a Proposal Please Confirm You Have Done The Following...

Description

One line of code triggers two different rules:

So if I must use @ts-ignore-error (due to flaky typescript errors) I have to disable two rules.

Why are there two rules that do the same thing? Given that prefer-ts-expect-error already exists, I think it would make sense to drop the specific check from ban-ts-comment

Impacted Configurations

I think one is in recommended and the other one is just in strict

Additional Info

No response

@fregante fregante added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin preset config change Proposal for an addition, removal, or general change to a preset config triage Waiting for maintainers to take a look labels Feb 1, 2024
@bradzacher
Copy link
Member

I think that this behaviour is still correct. In most code any form of TS comment is a code smell.

So having an error to warn against using @ts-expect-error is good practice.

Having an error to warn against @ts-ignore is good too - because it doesn't error if it suppresses nothing.

Think of it this way: the rules are saying "hey 100% definitely do not use @ts-ignore, prefer @ts-expect-error but also you shouldn't use that unless you really mean it".

If you use the "correct" suppression you only need one disable comment to say "yeah I'm doing this on purpose". If you use the "incorrect" one you need two to say "I'm really, really doing this on purpose and I know I'm breaking the rules".

@bradzacher bradzacher added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for maintainers to take a look labels Feb 4, 2024
@fregante
Copy link
Contributor Author

fregante commented Feb 4, 2024

No, I mean the rules do exactly the same thing, they both warn on @ts-ignore-error.

@ts-expect-error is perfectly allowed by both rules

@bradzacher
Copy link
Member

I see how that ban-ts-comment actually includes a special message to suggest using ts-expect-error if you use ts-ignore.

So you are indeed correct - by default prefer-expect-error is a subset of ban-ts-comment.

We should just remove the former entirely.

Cc @typescript-eslint/triage-team

@bradzacher bradzacher added accepting prs Go ahead, send a pull request that resolves this issue and removed awaiting response Issues waiting for a reply from the OP or another party preset config change Proposal for an addition, removal, or general change to a preset config labels Feb 4, 2024
@bradzacher bradzacher added this to the 7.0.0 milestone Feb 4, 2024
@Josh-Cena
Copy link
Member

I had the idea too; I just thought prefer-expect-error had a very educative and actionable error message/name.

@bradzacher
Copy link
Member

bradzacher commented Feb 4, 2024

Yeah the name is nice but ban-ts-comment has the same error message.

@JoshuaKGoldberg
Copy link
Member

+1 to this: if there's anything prefer-expect-error does better around messaging, part of the deprecation should involve moving that over to ban-ts-comment.

@auvred
Copy link
Member

auvred commented Feb 4, 2024

Yep, ban-ts-comment basically does the same, with one difference: prefer-expect-error provides autofix, while ban-ts-comment provides editor suggestion, which I think is nicer!

But while looking through the ban-ts-comment code, I noticed one possibly blocking thing: it doesn't report on multiline block comments, while prefer-expect-error does:

Assume we have the following code:

/**  some text here
   @ts-ignore */
const foo: string = 1

@typescript-eslint/prefer-ts-expect-error reports ✅ playground

@typescript-eslint/ban-ts-comment doesn't report ❌ playground

I think ban-ts-comment should cover this case to completely replace prefer-ts-expect-error.

Filed #8368

@JoshuaKGoldberg JoshuaKGoldberg modified the milestones: 7.0.0, 8.0.0 Mar 8, 2024
@JoshuaKGoldberg JoshuaKGoldberg changed the title Configs: Drop redundant "ts-expect-error" check from ban-ts-comment Rules: Deprecate prefer-ts-expect-error in favor of ban-ts-comment Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepting prs Go ahead, send a pull request that resolves this issue package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
5 participants