Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

timkraut
Copy link
Contributor

Hi,

this PR adds an option to allow a single extend (option:allowSingleExtend: true|false) to the ruleno-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:

constProductTile<ProductTileProps>=()=>{// Component implementation}

Problem

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

constProductTile<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 extendProduct (which doesn't make sense asonClick doesn't belong to aProduct) or replace the interface.

Solution

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

interfaceProductTilePropsextendsProduct{}constProductTile<ProductTileProps>=()=>{// Component implementation}

Now if we add an additional prop likeonClick, we could simply add it to the interfaceProductTileProps and we're done:

constProductTilePropsextendsProduct{onClick?(event:MouseEvent<HTMLButtonElement>):void}// Component stays 100% the same

This PR allows to do that while still warning if empty interfaces likeinterface 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!

j-f1 and maxmilton reacted with thumbs up emoji
@timkrauttimkrautforce-pushed theno-empty-interface-config-extends branch fromd70553b to5cf9322CompareFebruary 5, 2019 14:02
@codecov
Copy link

codecovbot commentedFeb 5, 2019
edited
Loading

Codecov Report

Merging#215 intomaster willincrease coverage by<.01%.
The diff coverage is100%.

@@            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 FilesCoverage Δ
...ages/eslint-plugin/src/rules/no-empty-interface.ts100% <100%> (ø)⬆️

@timkrauttimkrautforce-pushed theno-empty-interface-config-extends branch from5cf9322 to59bae94CompareFebruary 5, 2019 15:16
@timkrauttimkrautforce-pushed theno-empty-interface-config-extends branch from59bae94 to9789843CompareFebruary 5, 2019 16:14
bradzacher
bradzacher previously approved these changesFeb 5, 2019
Copy link
Member

@bradzacherbradzacher 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 👍

timkraut reacted with thumbs up emoji
armano2
armano2 previously approved these changesFeb 5, 2019
JamesHenry
JamesHenry previously requested changesFeb 6, 2019
@bradzacherbradzacher changed the titlefeat(no-empty-interface): add allowSingleExtend optionfeat(eslint-plugin): no-empty-interface - add allowSingleExtend optionFeb 7, 2019
@JamesHenryJamesHenry dismissedbradzacher’sstale reviewFebruary 7, 2019 13:08

Brad seems to agree with my suggested name change

@timkrauttimkrautforce-pushed theno-empty-interface-config-extends branch 2 times, most recently from758bdfb toab970d3CompareFebruary 8, 2019 12:40
bradzacher
bradzacher previously approved these changesFeb 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
ContributorAuthor

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

@bradzacherbradzacher added the enhancement: new plugin ruleNew rule request for eslint-plugin labelFeb 11, 2019
@bradzacherbradzacher merged commitbf46f8c intotypescript-eslint:masterFeb 14, 2019
@timkrauttimkraut deleted the no-empty-interface-config-extends branchFebruary 15, 2019 08:39
@github-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsApr 21, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@JamesHenryJamesHenryJamesHenry approved these changes

@bradzacherbradzacherbradzacher approved these changes

@armano2armano2armano2 left review comments

Assignees
No one assigned
Labels
enhancement: new plugin ruleNew rule request for eslint-plugin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@timkraut@bradzacher@armano2@JamesHenry

[8]ページ先頭

©2009-2025 Movatter.jp