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.

Sign up for GitHub

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

Jump to bottom

[Typescript] Lookup for external modules in @types folder. #1526

Merged
merged 1 commit into from
Jan 1, 2020
Merged

[Typescript] Lookup for external modules in @types folder. #1526

merged 1 commit into from
Jan 1, 2020

Conversation

joaovieira
Copy link
Contributor

@joaovieira joaovieira commented Oct 30, 2019 •
edited by ljharb
Loading

Why

When using TypeScript an external module can be resolved to its type definition file located in node_modules/@types from [email protected]. At the moment, that module would be marked as "internal" because the module is not found directly inside node_modules, but instead nested inside @types.

What

This PR just adds the node_modules/@types folder to lookup for external modules when using the TypeScript config.

Fixes #1525. Fixes #1541.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 96.243% when pulling 4fb469b on joaovieira:support-ts-resolver-v2 into 112a0bf on benmosher:master.

4 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 96.243% when pulling 4fb469b on joaovieira:support-ts-resolver-v2 into 112a0bf on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 96.243% when pulling 4fb469b on joaovieira:support-ts-resolver-v2 into 112a0bf on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 96.243% when pulling 4fb469b on joaovieira:support-ts-resolver-v2 into 112a0bf on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.3%) to 96.243% when pulling 4fb469b on joaovieira:support-ts-resolver-v2 into 112a0bf on benmosher:master.

@coveralls
Copy link

coveralls commented Oct 30, 2019 •
edited
Loading

Coverage Status

Coverage decreased (-0.05%) to 96.305% when pulling ae747c0 on joaovieira:support-ts-resolver-v2 into 078b6f7 on benmosher:master.

@joaovieira joaovieira mentioned this pull request Oct 30, 2019
Closed
JounQin
JounQin approved these changes Oct 30, 2019
View reviewed changes
Copy link
Collaborator

@JounQin JounQin left a comment

Choose a reason for hiding this comment

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

Great, didn't known about this option before.

@JounQin JounQin mentioned this pull request Oct 31, 2019
Closed
@joaovieira
Copy link
Contributor Author

@benmosher @ljharb any time to review this one liner?

(and maybe while you're at it, also #1528. These two are breaking for TS users with the new [email protected]).

ljharb
ljharb reviewed Dec 5, 2019
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

Any chance we could get a test for this?

@joaovieira
Copy link
Contributor Author

@ljharb I assumed we didn't test configs, as there are no tests for any of them.

The import/external-module-folders setting has already been tested in https://github.com/benmosher/eslint-plugin-import/blob/master/tests/src/core/importType.js#L144.

Happy to test that the config sets that setting?

@ljharb
Copy link
Member

ljharb commented Dec 14, 2019

@joaovieira while that is quite true that we don't already (my bad) it's still better to add one, as if something's not tested, that tells me it's ok to break :-)

@ljharb ljharb force-pushed the support-ts-resolver-v2 branch 2 times, most recently from 7cec495 to 34b1574 Compare December 30, 2019 08:05
@ljharb
Copy link
Member

ljharb commented Dec 30, 2019

I'm not sure why the travis tests are failing; this test passes for me locally.

@joaovieira
Copy link
Contributor Author

@ljharb fixed. Mocha doesn't seem to play nicely with globs. Decided to put the config tests next to all the others to simplify.

@ljharb ljharb merged commit ae747c0 into import-js:master Jan 1, 2020
@thewilkybarkid thewilkybarkid mentioned this pull request Jan 30, 2020
Closed
@ljharb ljharb mentioned this pull request Feb 3, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JounQin JounQin JounQin approved these changes

@noahbenham noahbenham noahbenham approved these changes

@ljharb ljharb ljharb approved these changes

Assignees
No one assigned
Labels
bug help wanted typescript
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cannot resolve node_modules/@types modules TypeScript import/order regression
5 participants
@joaovieira @coveralls @ljharb @noahbenham @JounQin