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

support export type named exports from typescript #1304

Merged
merged 4 commits into from Apr 10, 2019

Conversation

Projects
None yet
5 participants
@bradennapier
Copy link
Contributor

bradennapier commented Mar 19, 2019

Adds support so that export type named imports work with this plugin.

Resolves #1282

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 19, 2019

Coverage Status

Coverage remained the same at 97.862% when pulling 49af9d8 on bradennapier:feature/typescript-export-type into 3aefa79 on benmosher:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 19, 2019

Coverage Status

Coverage increased (+3.2%) to 97.336% when pulling 4d93618 on bradennapier:feature/typescript-export-type into 206d676 on benmosher:master.

2 similar comments
@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 19, 2019

Coverage Status

Coverage increased (+3.2%) to 97.336% when pulling 4d93618 on bradennapier:feature/typescript-export-type into 206d676 on benmosher:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 19, 2019

Coverage Status

Coverage increased (+3.2%) to 97.336% when pulling 4d93618 on bradennapier:feature/typescript-export-type into 206d676 on benmosher:master.

@ljharb
Copy link
Collaborator

ljharb left a comment

Thanks! Can you please add some test cases?

@ljharb ljharb added the typescript label Mar 21, 2019

@kjleitz

This comment has been minimized.

Copy link

kjleitz commented Mar 27, 2019

@ljharb I was checking out the bug this fixes, and I'm just curious—what would a test case look like for this? I'm not sure I see any existing examples of specs for the other named declaration exports (just grepped for a few of them)

@bradennapier

This comment has been minimized.

Copy link
Contributor Author

bradennapier commented Mar 27, 2019

Yeah I’ve been meaning to ask that. In fact it would seem this somehow increased coverage although not sure how or why.

It’s a single case statement added and only covers a typescript type alias.

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Mar 28, 2019

The OP in the linked issue seems to have a test case?

schmod added a commit to schmod/eslint-plugin-import that referenced this pull request Apr 1, 2019

@schmod

This comment has been minimized.

Copy link
Contributor

schmod commented Apr 1, 2019

Here's a commit that adds test-coverage. (GitHub won't let me PR it directly into @bradennapier's branch for some reason)

This turned out to be a weird one -- there are already tests for this functionality, but they were passing. The root issue actually appears to be a regression that only appears when switching from typescript-eslint-parser to @typescript-eslint/parser. I've updated the tests to run against both parsers.

typescript-eslint-parser was recently deprecated, but is widely-used to the extent where we should avoid deliberately breaking support for it in the short-term.

@ljharb ljharb force-pushed the bradennapier:feature/typescript-export-type branch from 4d93618 to 035ddb3 Apr 1, 2019

ljharb added a commit to bradennapier/eslint-plugin-import that referenced this pull request Apr 1, 2019

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Apr 1, 2019

@schmod thanks; i've added that commit to this branch.

Show resolved Hide resolved package.json

@ljharb ljharb force-pushed the bradennapier:feature/typescript-export-type branch from 035ddb3 to c3b5707 Apr 2, 2019

ljharb added a commit to bradennapier/eslint-plugin-import that referenced this pull request Apr 2, 2019

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Apr 2, 2019

@schmod looks like tests are failing in eslint 2 and 3.

@schmod

This comment has been minimized.

Copy link
Contributor

schmod commented Apr 3, 2019

Huh. I'm guessing that we've uncovered an existing incompatibility between @typescript-eslint/parser and eslint 3.

Given that @typescript-eslint/parser has a peerDependency on eslint 5.x, I guess we'll just want to skip any tests using that parser on ESLint <5.

@schmod

This comment has been minimized.

Copy link
Contributor

schmod commented Apr 3, 2019

@ljharb ljharb force-pushed the bradennapier:feature/typescript-export-type branch from c3b5707 to eeff95f Apr 6, 2019

ljharb added a commit to bradennapier/eslint-plugin-import that referenced this pull request Apr 6, 2019

@ljharb

ljharb approved these changes Apr 6, 2019

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Apr 6, 2019

@schmod eslint 2 and 3 tests are still failing with that commit :-/

@schmod

This comment has been minimized.

Copy link
Contributor

schmod commented Apr 6, 2019

Huh, I'll have to dig in. They passed on my local machine.

@schmod

This comment has been minimized.

Copy link
Contributor

schmod commented Apr 8, 2019

Fun times. The tests pass on MacOS (using the dep-time-travel.sh script and all), but fail on Linux/Docker, and I have no clue why.

I've narrowed this down to @eslint-typescript/parser merely being installed (which shouldn't matter?!). I'll add a workaround to dep-time-travel to remove the new parser when it isn't being used by the tests (ie. on ESLint <5), but this is a pretty spooky bug, TBH.

Commit: 1914fd6

@ljharb ljharb force-pushed the bradennapier:feature/typescript-export-type branch from eeff95f to 49af9d8 Apr 10, 2019

@ljharb ljharb merged commit 28dd614 into benmosher:master Apr 10, 2019

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.6%) to 97.862%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.