Skip to content

feat(eslint-plugin): no-empty-interface - add allowSingleExtend option #215

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

Merged

Conversation

timkraut
Copy link
Contributor

@timkraut timkraut commented Feb 5, 2019

Hi,

this PR adds an option to allow a single extend (option: allowSingleExtend: true|false) to the rule no-empty-interface. Our use case for this is the following:

Use case

We are building a React application with TypeScript. In React, you typically have components which might look like this:

const ProductTile<ProductTileProps> = () => {
  // Component implementation
}

Problem

Let's say our <ProductTile> component accepts the exact same props as the members defined in an interface Product { ... }. We could change the definition of the <ProductTile> component as follows to support that:

const ProductTile<Product> = () => {
  // Component implementation
}

But once we have to add an additional prop (e.g. onClick?(event: MouseEvent<HTMLButtonElement>): void), we have the issue that we would have to extend Product (which doesn't make sense as onClick doesn't belong to a Product) or replace the interface.

Solution

Instead, we would prefer to define the component as follows initially:

interface ProductTileProps extends Product {}

const ProductTile<ProductTileProps> = () => {
  // Component implementation
}

Now if we add an additional prop like onClick, we could simply add it to the interface ProductTileProps and we're done:

const ProductTileProps extends Product {
  onClick?(event: MouseEvent<HTMLButtonElement>): void
}

// Component stays 100% the same

This PR allows to do that while still warning if empty interfaces like interface Foo {} are used. It's an optional configuration property so that the default behavior is not changed.

P.S.: This is my first PR to change an ESLint-style rule. Let me know if there is anything I should do differently. Thanks a lot!

@timkraut timkraut force-pushed the no-empty-interface-config-extends branch from d70553b to 5cf9322 Compare February 5, 2019 14:02
@codecov
Copy link

codecov bot commented Feb 5, 2019

Codecov Report

Merging #215 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #215      +/-   ##
==========================================
+ Coverage   94.63%   94.63%   +<.01%     
==========================================
  Files          66       66              
  Lines        2794     2796       +2     
  Branches      723      724       +1     
==========================================
+ Hits         2644     2646       +2     
  Misses         57       57              
  Partials       93       93
Impacted Files Coverage Δ
...ages/eslint-plugin/src/rules/no-empty-interface.ts 100% <100%> (ø) ⬆️

@timkraut timkraut force-pushed the no-empty-interface-config-extends branch from 5cf9322 to 59bae94 Compare February 5, 2019 15:16
@timkraut timkraut force-pushed the no-empty-interface-config-extends branch from 59bae94 to 9789843 Compare February 5, 2019 16:14
bradzacher
bradzacher previously approved these changes Feb 5, 2019
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

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

code LGTM.
Good work on your first contribution 👍

armano2
armano2 previously approved these changes Feb 5, 2019
@bradzacher bradzacher changed the title feat(no-empty-interface): add allowSingleExtend option feat(eslint-plugin): no-empty-interface - add allowSingleExtend option Feb 7, 2019
@JamesHenry JamesHenry dismissed bradzacher’s stale review February 7, 2019 13:08

Brad seems to agree with my suggested name change

@timkraut timkraut force-pushed the no-empty-interface-config-extends branch 2 times, most recently from 758bdfb to ab970d3 Compare February 8, 2019 12:40
bradzacher
bradzacher previously approved these changes Feb 8, 2019
@bradzacher
Copy link
Member

Hey @timkraut, sorry for the pain, but we just merged a big PR to convert the plugin to typescript.
Please have a look through the source code and migrate your PR to typescript as well.
Let me know if you've got any questions!

@timkraut
Copy link
Contributor Author

@bradzacher Don't worry, it totally makes sense to use TypeScript in this case. I've converted the PR to TypeScript. Please check again

@bradzacher bradzacher added the enhancement: new plugin rule New rule request for eslint-plugin label Feb 11, 2019
@bradzacher bradzacher merged commit bf46f8c into typescript-eslint:master Feb 14, 2019
@timkraut timkraut deleted the no-empty-interface-config-extends branch February 15, 2019 08:39
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement: new plugin rule New rule request for eslint-plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants