8000 [ban-types] Should not ban `{}` by default! · Issue #2063 · typescript-eslint/typescript-eslint · GitHub
[go: up one dir, main page]
Skip to content

[ban-types] Should not ban {} by default! #2063

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.

Sign up for GitHub

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

Jump to bottom
Closed
Mister-Hope opened this issue May 22, 2020 · 25 comments
Closed

[ban-types] Should not ban {} by default! #2063

Mister-Hope opened this issue May 22, 2020 · 25 comments
Labels
package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin wontfix This will not be worked on working as intended Issues that are closed as they are working as intended

Comments

@Mister-Hope
Copy link
Contributor
Mister-Hope commented May 22, 2020 •
edited
Loading

A lot of type declaration is using Type Aliases, to let user fill in.

For example a interface like this: (I got it from an official repo)

  interface Custom<
    Detail = Record<string, any>,
    TargetDataset = Record<string, any>,
    CurrentTargetDataset = Record<string, any>,
    Mark = Record<string, any>
  > extends Base<TargetDataset, CurrentTargetDataset, Mark> {
    detail: Detail;
  }

Sometimes detail is just an empty object! So it's good for me to write Custom<{}>, but V3 will throw an error ( not even a warning)

Don't use `{}` as a type. `{}` actually means "any non-nullish value".
- If you want a type meaning "any object", you probably want `Record<string, unknown>` instead.
- If you want a type meaning "any value", you probably want `unknown` instead.

In order to stop this error, I have to write Custom<Record<never,never>>, which is unneeded and a waste of time.

So stop baning {} by default. May packages are holding certain objects for certain data, and to prevent the Cannot read property 'xxx' of undefined, they are usually initialize with {}, so If I know that those fields are not containing any data, I should use {}
@Mister-Hope Mister-Hope added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels May 22, 2020
@TrejGun

This comment has been minimized.

Sign in to view
@bbarry

This comment has been minimized.

Sign in to view
@Mister-Hope

This comment has been minimized.

Sign in to view
@Mister-Hope

This comment has been minimized.

Sign in to view
@bbarry

This comment has been minimized.

Sign in to view
@Mister-Hope

This comment has been minimized.

Sign in to view
@bbarry

This comment has been minimized.

Sign in to view
@bradzacher
Copy link
Member
bradzacher commented May 22, 2020 •
edited
Loading

See comment at the end of this post: #2063 (comment)

@bradzacher bradzacher added working as intended Issues that are closed as they are working as intended and removed triage Waiting for team members to take a look labels May 22, 2020
@bbarry

This comment has been minimized.

Sign in to view
@mgcrea

This comment has been minimized.

Sign in to view
@bbarry

This comment has been minimized.

Sign in to view
@zetoke

This comment has been minimized.

Sign in to view
@nstepien

This comment has been minimized.

Sign in to view
@bbarry

This comment has been minimized.

Sign in to view
@nstepien

This comment has been minimized.

Sign in to view
@bbarry

This comment has been minimized.

Sign in to view
@phiresky

This comment has been minimized.

Sign in to view
@bradzacher

This comment has been minimized.

Sign in to view
@nikklassen nikklassen mentioned this issue Jun 20, 2020
Merged
@leethree

This comment has been minimized.

Sign in to view
@bbarry

This comment has been minimized.

Sign in to view
@leethree

This comment has been minimized.

Sign in to view
@TellonUK TellonUK mentioned this issue Jul 11, 2020
Open
@msakrejda

This comment has been minimized.

Sign in to view
@msakrejda

This comment has been minimized.

Sign in to view
@misuken-now

This comment has been minimized.

Sign in to view
@bradzacher bradzacher added the wontfix This will not be worked on label Aug 17, 2020
@typescript-eslint typescript-eslint locked as resolved and limited conversation to collaborators Aug 17, 2020
@bradzacher
Copy link
Member
bradzacher commented Aug 17, 2020 •
edited
Loading

We will not be removing {} from the rule defaults, as it is an unsafe type because it doesn't work how people think it works, and in most cases it allows weakly typed code.

This is the exact reason that the rule bans the {} type.
It's a common misconception that the {} type is the same as the {} value.
But as the rule's message states: this is not the case!

The type {} doesn't mean "any empty object", it means "any non-nullish value".

This is obviously a huge type safety hole!

For example - the following code is completely type-check valid, even though it might make no sense to be:

interface AAA {
  aaa: {};
}

const x: AAA = { aaa: true };

repl

It's also important to note that empty interfaces behave in exactly the same way as the {} type!, which is why we have the no-empty-interface lint rule.

Unfortunately, there's no type in TS that means "an empty object".

There are the following options for you:

If you want a type that means "empty object"

You can use a type similar to this type.

type EmptyObject = Record<string, never>; // or {[k: string]: never}

const a: EmptyObject = { a: 1 };  // expect error
const b: EmptyObject = 1;         // expect error
const c: EmptyObject = () => {};  // expect error
const d: EmptyObject = null;      // expect error
const e: EmptyObject = undefined; // expect error
const f: EmptyObject = {};        // NO ERROR - as expected

ts playground repl

If you are using React, and you want to define type Props = {}.

This is technically safe in this instance, because under the hood the {} type is passed into an intersection type (see the note at the end of this comment for why this is safe).

However, there is no way for us to statically analyse and know that this is a safe usage.
To work around this, consider reconfiguring the lint rule to match your repository's coding style.
You can use the following config to allow it:

{
  "rules": {
    "@typescript-eslint/ban-types": [
      "error",
      {
        "extendDefaults": true,
        "types": {
          "{}": false
        }
      }
    ]
  }
}

Consider using an eslint overrides config to limit the scope of this change to just react component files, to help ensure you're keeping your codebase as safe as possible.

As an aside - it's worth noting that {} is a very weird anomaly in TypeScript, because there is just one case where it actually does mean something akin to "empty object"; in an intersection type.

type T1 = { a: 1 } & {};
const t11: T1 = { a: 1 };
const t12: T1 = true; // expected error

type T2 = true & {};
const t21: T2 = true;
const t22: T2 = false; // expected error
const t23: T2 = {}; // expected error

repl

In this usage, the type essentially is a no-op, and means nothing at all.

In all other usages, including in the extends clause of a generic type parameter, it means "anything non-nullish".

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin wontfix This will not be worked on working as intended Issues that are closed as they are working as intended
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
0