Skip to content

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

Merged
ljharb merged 1 commit into
import-js:masterfrom
joaovieira:support-ts-resolver-v2
Jan 1, 2020
Merged

[Typescript] Lookup for external modules in @types folder.#1526
ljharb merged 1 commit into
import-js:masterfrom
joaovieira:support-ts-resolver-v2

Conversation

@joaovieira
Copy link
Copy Markdown
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 eslint-import-resolver-typescript@2.0.0. 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
Copy Markdown

coveralls commented Oct 30, 2019

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
Copy Markdown

coveralls commented Oct 30, 2019

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
Copy Markdown

coveralls commented Oct 30, 2019

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
Copy Markdown

coveralls commented Oct 30, 2019

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
Copy Markdown

coveralls commented Oct 30, 2019

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
Copy Markdown

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
Copy Markdown
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
Copy Markdown
Contributor Author

joaovieira commented Nov 11, 2019

@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 eslint-import-resolver-typescript@2.0.0).

ljharb
ljharb reviewed Dec 5, 2019
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

joaovieira commented Dec 13, 2019

@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
Copy Markdown
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 from f46dc45 to 64c0823 Compare December 29, 2019 23:30
@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
Copy Markdown
Member

ljharb commented Dec 30, 2019

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

@joaovieira joaovieira force-pushed the support-ts-resolver-v2 branch from d82d356 to f134475 Compare December 31, 2019 13:38
@joaovieira
Copy link
Copy Markdown
Contributor Author

joaovieira commented Dec 31, 2019

@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 force-pushed the support-ts-resolver-v2 branch from f134475 to ae747c0 Compare January 1, 2020 19:00
@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

@ljharb ljharb ljharb approved these changes

+2 more reviewers

@noahbenham noahbenham noahbenham approved these changes

@JounQin JounQin JounQin approved these changes

Reviewers whose approvals may not affect merge requirements

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