Ignore type imports for named rule.

[mattijsbliek]

This fixes #931 for the named rule. This was done to play nice with flow-typed in the following situation:

/*
 * Import an NPM module and grab the type definition from flow-typed
 */
import Something from 'my-npm-package';
// This type isn’t actually in the npm package but in the flow-typed directory, so eslint-import cannot resolve it.
import type { SomethingType } from 'my-npm-package';

Closes #1060.

[coveralls]

Coverage remained the same at 96.479% when pulling 1a76c52 on mattijsbliek:ignore-type-imports-for-named-rule into cfd4377 on benmosher:master.

[ljharb]

Hmm. This actually seems to defeat the purpose of this very rule; instead of ignoring types here, is there a way we could resolve them the same way flow itself does?

[mattijsbliek]

Absolutely, this is not ideal. Flow can resolve the imports because it checks the directories listed under [libs] in .flowconfig. Ideally you would just add the flow-typed directory to the modules array of import/resolver’s config options. The reason this doesn’t work is because the location of the typed file does not map 1:1 to the import:

// Import from 'bar'
import type { Foo } from 'bar';

// Actual typed file is here:
flow-typed/npm/bar_vx.x.x.js

// And type looks like this
declare module 'bar' {
  declare export type Foo = 'TYPE_HERE';
}

I think Flow just goes through all the files in the flow-typed directory and accepts the declared modules. I’m not sure there would be a way for ESLint Plugin Import to understand what is going on without doing the same thing. But this would mean it would have to parse Flow syntax and everything, which I think is not something we want.

Not sure how to otherwise solve this if not for just skipping typed imports. What do you think?

[ljharb]

The current scenario is that the plugin correctly verifies some types, but falsely reports on those in flow-typed - this change removes the usefulness in the former case, correct?

[mattijsbliek]

Correct! The plugin can verify all types that are just normally exported from a file.

[ljharb]

What about if we did this instead: if the file being imported exports types, then do the checking - if it does not, then for now, ignore it.

[mattijsbliek]

Nice, that could work. I’ll try and implement this today.

[mattijsbliek]

Spent the past 4 hours trying to get the export types from the imported file, but couldn’t figure it out. I’m not very familiar with ASTs or ESLint, could you point me in the right direction? If it’s easier for someone else to take over I can create a new issue as well.

[tf]

Why does ESLint need to check type imports at all? If we are importing types, doesn't that mean Flow is already in the picture and will check things itself? The only case I currently see that is handled by this plugin but not by Flow is when importing things from a non-Flow file. But then we cannot import types anyway. What am I missing?

[mattijsbliek]

I do think that’s a good argument, Flow indeed already does the checking for missing imported types. What are your thoughts on this @ljharb?

[ljharb]

Hmm, that's a fair point.

Let's keep this as-is for now - the PR needs a fresh rebase tho.

[tf]

Looking at the overall structure of the test, I suggest moving this into the group of valid examples, instead of having it under invalid but with no errors.

If we'll just ignore type imports, I'd even go a step further and remove the remaining valid import type examples since they convey the impression that the plugin still cares about whether a type import is valid or not.

[mattijsbliek]

Good suggestions, I made it so.

[benmosher]

LGTM, simpler!

[syymza]

This seems to fix #921. 🎺

Can we have a new release including it?

[benmosher]

I thought this was released in the last release but perhaps not?

[syymza]

@benmosher this was merged 18 days ago. The last release was 19 days ago.

v2.11.0...master

[benmosher]

wah wah 😅 my bad. was hoping for this morning but burned all my time on something else. still likely to be this week.

[syymza]

Knock Knock... :)

[benmosher]

just published! 😅

[chadlwilson]

Thanks @benmosher - looks to be working great! Just removed a whole lot of nasty // eslint-disable-nextline import/nameds :)