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

💅 lint/suspicious/noEmptyInterface false positive #1157

Closed
1 task done
jer-sen opened this issue Dec 12, 2023 · 13 comments · Fixed by #1289
Closed
1 task done

💅 lint/suspicious/noEmptyInterface false positive #1157

jer-sen opened this issue Dec 12, 2023 · 13 comments · Fixed by #1289

Comments

@jer-sen
Copy link

jer-sen commented Dec 12, 2023

Environment information

CLI:
  Version:                      1.4.1
  Color support:                true

Platform:
  CPU Architecture:             x86_64
  OS:                           windows

Environment:
  BIOME_LOG_DIR:                unset
  NO_COLOR:                     unset
  TERM:                         "xterm-256color"
  JS_RUNTIME_VERSION:           "v20.10.0"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         "yarn/1.22.21"

Biome Configuration:
  Status:                       Loaded successfully
  Formatter disabled:           false
  Linter disabled:              false
  Organize imports disabled:    false
  VCS disabled:                 true

Workspace:
  Open Documents:               0

Rule name

lint/suspicious/noEmptyInterface

Playground link

https://biomejs.dev/playground/?lintRules=all&code=aQBuAHQAZQByAGYAYQBjAGUAIABCACAAewAKACAAIABhADoAIABiAG8AbwBsAGUAYQBuACwACgAgACAAYgA6ACAAYgBvAG8AbABlAGEAbgAsAAoAfQAKAGkAbgB0AGUAcgBmAGEAYwBlACAAQQAgAGUAeAB0AGUAbgBkAHMAIABQAGkAYwBrADwAQgAsACAAJwBhACcAPgAgAHsAfQA%3D

Expected result

Should not report an error when extending something since the result is not an empty interface.

Code of Conduct

  • I agree to follow Biome's Code of Conduct
@Conaclos
Copy link
Member

Conaclos commented Dec 12, 2023

noEmptyInterface also reports empty interfaces that extend a type as showed in the diagnostic:

main.tsx:5:1 [lint/suspicious/noEmptyInterface](https://biomejs.dev/linter/rules/no-empty-interface)  FIXABLE  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

  ✖ An interface declaring no members is equivalent to its supertype.
  
    3 │   b: boolean,
    4 │ }
  > 5 │ interface A extends Pick<B, 'a'> {}
      │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  
  ℹ Safe fix: Convert empty interface to type alias.
  
    3 3 │     b: boolean,
    4 4 │   }
    5   │ - interface·A·extends·Pick<B,·'a'>·{}
      5 │ + type·A·=·Pick<B,·'a'>

@jer-sen
Copy link
Author

jer-sen commented Dec 12, 2023

What's your point?

Type alias is not the same than an interface.
Cf https://www.sitepoint.com/typescript-type-vs-interface/

@Conaclos
Copy link
Member

We implemented the same behavior as the TypeScript Eslint rule no-empty-interface.

Why a type alias doesn't fit your use-case?

@jer-sen
Copy link
Author

jer-sen commented Dec 12, 2023

We try to always use interfaces cf https://typescript-eslint.io/rules/consistent-type-definitions
For consistency, performance and inconsistency detection when declaring interface (instead of getting a never type detected where it is used).

@Conaclos
Copy link
Member

Conaclos commented Dec 12, 2023

For consistency, performance

Performance?

Are you aware that Pick is defined with a type alias? This seems really questionable that an interface extends a type alias...

Moreover, I think that an empty interface that extends a type is non-idiomatic TypeScript code. Even the TypeScript team that advocates the use of interfaces, resort to type aliases for this kind of thing.

@jer-sen
Copy link
Author

jer-sen commented Dec 13, 2023

@Conaclos
Copy link
Member

Conaclos commented Dec 13, 2023

Thanks for sharing this resource!

They recommend using interfaces that extend two or more types instead of using type intersections. Thus, this doesn't apply to empty interfaces that extend a single type. Although not explicitly said, I think the TypeScript team recommend using type aliases instead of interfaces when the interface is empty and extends a single type. This is why the TypeScript ESLint rule no-empty-interface only recommend removing empty interface that extends zero or a single type.

If you are always using interfaces over type aliases, I think you should disable the noEmptyInterface rule?

@blutorange
Copy link

blutorange commented Dec 17, 2023

Another case where you must use an interface: When you need to augment an interface. With the infamous JQuery as an example:

declare global {
  interface JQuery extends JQueryFunctionExtensionsMyAwesomePlugin {}
}

The linter would rewrite the above as

declare global {
  type JQuery = JQueryFunctionExtensionsMyAwesomePlugin;
}

which does not augment the module, and is also a compile error. (Cannot augment module 'JQuery' with value exports because it resolves to a non-module entity.ts(2649))

Or a self contained example:

interface Foobar {
  foo: () => void;
  bar: () => void;
}
interface Baz {
  baz: () => void;
}
interface Foobar extends Baz {}

Replacing the last last line with type Foobar = Baz changes the semantics and also results in a compile error.

@Conaclos
Copy link
Member

Conaclos commented Dec 17, 2023

@blutorange
I recently fixed this for external module (See #959). Unfortunately I didn't think about global declarations :/

I opened #1243 to allow empty interfaces that extend a type in global declarations.

@blutorange
Copy link

blutorange commented Dec 17, 2023

@Conaclos That sounds like a good idea, and should fix that use case, thanks for the PR : )

Out of curiosity, how do you feel about allowing empty interfaces that extend something in general, not only in module / global declarations? Code such as this is possible, albeit I'd hope less common (personally I don't think I'd need it):

interface Foo {
  x: () => void;
}

interface Bar {
  y: () => void;
}

interface Baz {
  z: () => void;
}

interface Foo extends Bar {}

One could argue that the interface is not empty when properties are pulled in via extends. It might also be a bit more understandable to users, since no error is reported if the number of extended interface is > 1:

// lint error (why do I have to change this to "type Foo = Bar" ?)
interface Foo extends Bar {}

// no lint error (why do I not have to change this to "type Foo = Bar & Baz" ?)
interface Foo extends Bar, Baz {}

@Conaclos
Copy link
Member

Yes, I have thought about this change. However, I am a bit hesitant about it because it will be a divergence from TypeScript ESLint. We could wait a bit to get more feedback.

@jer-sen
Copy link
Author

jer-sen commented Dec 19, 2023

I think the (not obvious) choice between type/interface should be handled by @typescript-eslint/consistent-type-definitions .
The noEmptyInterface should prevent use of {} when the developer wants an "empty" object.

Some other interesting content:

@Conaclos
Copy link
Member

The rule is now ignoring interfaces that extend a type. Thanks for your valuable comments!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants