Eslint-plugin-import: `no-unused-modules` does not work properly with flow types exports

[yamov]

This PR introduces support for Flow type exports https://github.com/benmosher/eslint-plugin-import/pull/1542

However, it only takes into account the identifier import style
https://github.com/benmosher/eslint-plugin-import/pull/1542/files#diff-48b646acd6be2c915b501dc667163bd4

And it does not work for the declaration import style. Example
Module a.js:

export type A = 'string'

Module b.js:

import type { A } from 'a.js'`

Will result in an error in module A:

ESLint: exported declaration 'A' not used within other modules(import/no-unused-modules)

Note:
It seems to work fine for me if I use identifier import style

import { type A } from 'a.js'`

[ljharb]

cc @rfermann

[rfermann]

Thanks for adding, @ljharb.

@yamov i‘m not familiar with flow at all. If you could provide all different ways of imports and exports particularly used by flow, I would be willing to (try to) add support for these.

[yamov]

@rfermann thanks a lot for your reply and for your effort.

Flow docs show these examples

exports.js

// @flow
export default class Foo {};
export type MyObject = { /* ... */ };
export interface MyInterface { /* ... */ };

imports.js

// @flow
import type Foo, {MyObject, MyInterface} from './exports';

AFAICS this doc is not complete (unfortunately, Flow is notorious for bad documentation 😞) and there can be these ways of importing Flow types:

import type { MyObject } from './exports'
````
and

import { type MyObject } from './exports'
````

Please let me know if I can provide any additional info.

[Hypnosphi]

The first step should probably be reverting #1542 and releasing a patch

[yamov]

👍

[rfermann]

While scanning the source code, i found this pull request. After commenting out all changes on ExportMap.js made by this PR, all examples provided by @yamov start to work (except the exported interface, which requires a modification of no-unused-modules).

After reading the related issue, there was a fix in place which has been reverted and replaced with the fix provided in the PR mentioned above.

@ljharb can we revert the PR #1494 and restore the original fix instead? This would help increasing the flow support for no-unused-modules.

[ljharb]

@rfermann sure, want to open up a PR that does that?

[rfermann]

Reluctantly. Just thought about it again. Maybe it's better to include @maxmalov, who created the other PR. He is familiar with the issue and might find another way to fix it.

@maxmalov could you please do a qick analysis if there are other ways to fix the original issue?

[maxmalov]

I guess this line causes the issue since it completely skips the whole module and that's why import type { A } from '...') reports an error.

On the other hand, it isn't clear to me why import { type A } from '...' works since imports with only flow types are skipped too here

Not sure I'll be able to do any quick investigations in a few weeks. It makes sense to revert #1494, but it worth adding some tests for this particular issue, so anyone can revisit fix for #1343 later

[rfermann]

On the other hand, it isn't clear to me why import { type A } from '...' works since imports with only flow types are skipped too here

That's because in this case specifier.importKind does not equal type. It equals value instead, hence it isn't catched.

Meanwhile i reverted the changes of #1493 locally but kept the tests, trying to make them still pass but with different changes on the source files. Only one is left so far, so if I am lucky, I will be able to provide a solution soon.

[maxmalov]

Make sure you’re using babel-eslint or flow parser https://astexplorer.net/#/gist/052189089ff2f468742815723f2dfeba/latest

Otherwise it isn’t possible to detect whether import specifier is a type or a value

[maxmalov]

Also, instead of removing only type imports from the dependency graph we can add some meta information to them, e.g typesOnly: true. This can be used in the cycle calculation phase later, and hopefully will fix no-unused-module rule. Since types module will exist in the dependency tree, but will be treated differently only in no-cycle rule.

[maxmalov]

Also, instead of removing only type imports from the dependency graph we can add some meta information to them

Btw, looks like this PR #1231 already do this, except import { type Foo } specifiers

[ljharb]

@maxmalov if you'd like to post a link to a branch on #1231, especially if you can also add test coverage, I'd be more than happy to pull them in and get that PR landed.

[maxmalov]

@ljharb the next week is pretty busy for me, but I guess I'll have some free time after that for a fix and test coverage

[rfermann]

@maxmalov thanks for linking to the other PR. Surprisingly I (partially) came to the same solution without even looking at this one :sweat_smile:

The downside is, this does not make all tests provided by @maxmalov pass. Although the last test for no-cycle is supposed to fail, it always fails with a wrong error message. It is failing in line 3, not in line 4 as supposed by the test.

But I am not sure if the test is supposing the correct line number in this case, as both lines are importing from the same file.
Can someone else being more familiar with this rule have a look at the test and tell me, if the test is assuming the correct line number to fail?

[maxmalov]

The downside is, this does not make all tests provided by @maxmalov pass. Although the last test for no-cycle is supposed to fail, it always fails with a wrong error message. It is failing in line 3, not in line 4 as supposed by the test

@rfermann it looks like the issue related to multiple imports from the same module. Consider the following example:

// @flow

import type { MyType } from './types';
import { type OtherType, myVariable } from './types';

The first is the import declaration with importKind: type. The second one is more complex, it is the import declaration without importKind, but it nests one import specifier with importKind: type. (AST explorer)

How I understand the current implementation, only the first import will be captured, leaving the second one untracked. This approach works for no-cycle rule in plain JS, since no matter how often the same module is being imported. But this makes things complicated since to make no-cycle rule work with flow ExportMap has to merge all kinds of multiple imports from the same module into a single record.

[Hypnosphi]

Seems fixed in 2.20

[Hypnosphi]

I overlooked something, it's not fixed yet. Sorry for confusion and please reopen

[Hypnosphi]

@maxmalov @rfermann
FYI: no-cycle probably shouldn't ignore Flow imports

https://github.com/benmosher/eslint-plugin-import/issues/1098#issuecomment-564805161

[maxmalov]

@Hypnosphi This was my original idea fix https://github.com/benmosher/eslint-plugin-import/issues/1564#issuecomment-565276520, but after a few tries I've ended up in a case when ExportMap is pretty hard to use in this solution

[Hypnosphi]

I see. Do you think #1542 can be reverted for now, until a proper solution is found? It fixed #1540 which is less critical than this one, at least for me

[Hypnosphi]

This was re-introduced in 2.22.0, maybe caused by https://github.com/benmosher/eslint-plugin-import/pull/1819

Actually, now even import { type A } from 'a.js' doesn't mark A as used

@ljharb please reopen

[ljharb]

@Hypnosphi sure. we should add some tests to prevent that from happening again. cc @nicolashenry

[nicolashenry]

I will take a look when I have more time but it seems that I may introduced wrongly TypeAlias and InterfaceDeclaration by babel-eslint as Typescript types when it seems to be Flow types. So some typescript tests which existed before my changes were probably relying wrongly on babel-eslint parser and I tried to make it compatible with it too but it was probably a bad idea unless some people are using babel-eslint parser with Typescript source code instead of @typescript-eslint/parser or typescript-eslint-parser.

I will try to change typescript tests to use typescript parsers only or maybe try to do a proper fix for Flow type imports but I am not a Flow user so I will may have some difficulties to do it.

[nicolashenry]

@Hypnosphi Can you provide me a test case? I can't reproduce using unit tests from https://github.com/benmosher/eslint-plugin-import/pull/1542 even adding import type { A } from 'a.js' test case.

[Hypnosphi]

@nicolashenry sure: https://github.com/Hypnosphi/import-flow-repro

[nicolashenry]

@Hypnosphi It seems to be related to the loading order, if I rename a.js to b.js, b.js to a.js and I change from './a' to from './b' in your repository I no longer have the error... I am not sure about how to fix this for now.

[nicolashenry]

@ljharb I created https://github.com/benmosher/eslint-plugin-import/pull/1906 to ignore Flow types again. I think babel-eslint parser causes an issue in module load ordering when flow type imports are used but I don't think that I can fix it (unless you have an idea on how to do it).