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

[bugfix] Re: #1332 - skip warning on a type declaration or alias. #1377

Merged

Conversation

sharmilajesupaul
Copy link
Contributor

@sharmilajesupaul sharmilajesupaul commented Jun 10, 2019

Fixes #1332

Skips warning on a single type declaration or type alias. Remove the check for node.exportKind as it is undefined in all test cases. I believe this is used in eslint v2 & 3

This should work with @typescript-eslint/parser too, but I'm not testing that here because this repo is still using typescript-eslint-parser.

cc. @ljharb

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 97.605% when pulling a686d0e on sharmilajesupaul:bugfix-prefer-default-export into 15e5c61 on benmosher:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 97.605% when pulling a686d0e on sharmilajesupaul:bugfix-prefer-default-export into 15e5c61 on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 97.605% when pulling a686d0e on sharmilajesupaul:bugfix-prefer-default-export into 15e5c61 on benmosher:master.

@coveralls
Copy link

coveralls commented Jun 10, 2019

Coverage Status

Coverage decreased (-0.1%) to 97.896% when pulling 3b21de6 on sharmilajesupaul:bugfix-prefer-default-export into 1caa402 on benmosher:master.

@sharmilajesupaul
Copy link
Contributor Author

I came across another the failing case with a single named export

i.e. this errors:

type foo = string;
export { foo }

could we merge those fixes in a separate PR?

cc. @ljharb

@ljharb
Copy link
Member

ljharb commented Jul 16, 2019

@sharmilajesupaul if this PR, once rebased, passes tests, then it's up to you if you want to defer those fixes to another PR - but it's also great if you can include them in this PR too :-)

@ljharb ljharb force-pushed the bugfix-prefer-default-export branch 3 times, most recently from 53f79e6 to 3ea8b4a Compare July 17, 2019 06:02
@ljharb
Copy link
Member

ljharb commented Jul 17, 2019

@sharmilajesupaul i've fixed the test issues around eslint and typescript parser versions; but now there's some seemingly legit failures. Can you look into them?

tests/src/rules/prefer-default-export.js Outdated Show resolved Hide resolved
tests/src/rules/prefer-default-export.js Outdated Show resolved Hide resolved
@sharmilajesupaul sharmilajesupaul force-pushed the bugfix-prefer-default-export branch 2 times, most recently from 078d712 to 79da3d1 Compare July 17, 2019 22:41
@ljharb ljharb force-pushed the bugfix-prefer-default-export branch from 79da3d1 to f4e3f1b Compare July 19, 2019 01:56
@ljharb ljharb merged commit 3b21de6 into import-js:master Jul 19, 2019
if (node.exportKind === 'type') return

if (
node.declaration.type === 'TSTypeAliasDeclaration' ||
node.declaration.type === 'TypeAlias'
Copy link
Contributor

Choose a reason for hiding this comment

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

@sharmilajesupaul @brieb should this also ignore interfaces? I believe the Babel/TypeScript AST type is InterfaceDeclaration and the typescript eslint parser type is TSInterfaceDeclaration.

Copy link
Contributor

Choose a reason for hiding this comment

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

lencioni added a commit to lencioni/eslint-plugin-import that referenced this pull request Jul 19, 2019
Similar to import-js#1377,
we don't want this rule to apply to exported interfaces.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

prefer-default-export fails on Typescript type declaration
4 participants