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

Enhancement: [ban-types] Allow {} in the recommended config #8697

Closed
4 tasks done
JoshuaKGoldberg opened this issue Mar 17, 2024 · 6 comments
Closed
4 tasks done

Enhancement: [ban-types] Allow {} in the recommended config #8697

JoshuaKGoldberg opened this issue Mar 17, 2024 · 6 comments
Labels
blocked by another issue Issues which are not ready because another issue needs to be resolved first enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commented Mar 17, 2024

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

My proposal is suitable for this project

  • I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Link to the rule's documentation

https://typescript-eslint.io/rules/ban-types

Description

The {} has long been a source of confusion in TypeScript. It means "any non-nullish value, but with no known fields". That includes objects like {} and { value: 1 }, primitives like true and "", arrays like [], and general class instances like new Error().

Because users so often get confused by {} allowing primitives like true and "", the ban-types rule suggests users use something more restrictive. Specifically its default options include:

`{}` actually means "any non-nullish value".',
- If you want a type meaning "any object", you probably want `object` instead.',
- If you want a type meaning "any value", you probably want `unknown` instead.',
- If you want a type meaning "empty object", you probably want `Record<string, never>` instead.',
- If you really want a type meaning "any non-nullish value", you probably want `NonNullable<unknown>` instead.',

That's pretty strict... at the cost of user convenience.

Now that #8364 is in, we have the ability to provide more lenient default options for the recommended* configs vs. the strict* configs.

Proposal: let's remove the restriction on {} from the default ban-types options in recommended configs, and leave them in for strict configs?

Fail

let value: Object;

Pass

let value: {};

Additional Info

@RyanCavanaugh, the dev lead for TypeScript, is not a fan of the way it is now 😄: https://bsky.app/profile/searyanc.dev/post/3knqtqcjhpn2d

See also #5947 for past discussion around objects and ban-types

@JoshuaKGoldberg JoshuaKGoldberg added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look enhancement: plugin rule option New rule option for an existing eslint-plugin rule labels Mar 17, 2024
@JoshuaKGoldberg
Copy link
Member Author

Just read microsoft/TypeScript#57735 and filed #8700 as a result. For clarity, the intent of the two issues I'm filing are:

If #8700 is resolved before this one, that's great too.

@bradzacher
Copy link
Member

bradzacher commented Mar 17, 2024

Strong 👎
See #8700 (comment)

(this issue and #8700 also seem like duplicates?)

@Josh-Cena
Copy link
Member

From what I understand: this is about removing the ban in recommended, while #8700 is about removing the ban everywhere...

@bradzacher
Copy link
Member

They're one and the same though. The rule supports banning {} but it only flags it because it's in the default config. If you remove it from the default config then it's removed from everywhere.

@JoshuaKGoldberg
Copy link
Member Author

As of #8364 we can have different default options for recommended and strict. That's what I meant for this issue.

But yeah, I wasn't sure whether to file this and/or #8700 as a single issue, or discussion, or etc.. Marking this one as blocked on resolving discussion there in the interim.

@JoshuaKGoldberg JoshuaKGoldberg added blocked by another issue Issues which are not ready because another issue needs to be resolved first and removed triage Waiting for team members to take a look labels Mar 18, 2024
@JoshuaKGoldberg
Copy link
Member Author

Closing per #8700 (comment). We can always revisit this if v9 shows that many users are still confused by the {} ban with the new, better-phrased rule.

@JoshuaKGoldberg JoshuaKGoldberg closed this as not planned Won't fix, can't repro, duplicate, stale Apr 23, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
blocked by another issue Issues which are not ready because another issue needs to be resolved first enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

No branches or pull requests

3 participants