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

no-unused-modules does not work properly with flow types exports #1564

Closed
yamov opened this issue Dec 10, 2019 · 29 comments · Fixed by #1770 or #1906
Closed

no-unused-modules does not work properly with flow types exports #1564

yamov opened this issue Dec 10, 2019 · 29 comments · Fixed by #1770 or #1906

Comments

@yamov
Copy link

yamov commented Dec 10, 2019

This PR introduces support for Flow type exports #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
Copy link
Member

ljharb commented Dec 10, 2019

cc @rfermann

@rfermann
Copy link
Contributor

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
Copy link
Author

yamov commented Dec 10, 2019

@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
Copy link
Contributor

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

@yamov
Copy link
Author

yamov commented Dec 11, 2019

👍

@rfermann
Copy link
Contributor

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
Copy link
Member

ljharb commented Dec 12, 2019

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

@rfermann
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

ljharb commented Dec 25, 2019

@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
Copy link
Contributor

@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
Copy link
Contributor

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

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
Copy link
Contributor

maxmalov commented Jan 5, 2020

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

This comment has been minimized.

@ljharb ljharb closed this as completed Jan 28, 2020
@Hypnosphi

This comment has been minimized.

@ljharb ljharb reopened this Jan 30, 2020
@Hypnosphi
Copy link
Contributor

Hypnosphi commented Feb 17, 2020

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

#1098 (comment)

@maxmalov
Copy link
Contributor

@Hypnosphi This was my original idea fix #1564 (comment), but after a few tries I've ended up in a case when ExportMap is pretty hard to use in this solution

@Hypnosphi
Copy link
Contributor

Hypnosphi commented Feb 18, 2020

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
Copy link
Contributor

This was re-introduced in 2.22.0, maybe caused by #1819

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

@ljharb please reopen

@ljharb
Copy link
Member

ljharb commented Sep 21, 2020

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

@ljharb ljharb reopened this Sep 21, 2020
@nicolashenry
Copy link
Contributor

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
Copy link
Contributor

@Hypnosphi Can you provide me a test case? I can't reproduce using unit tests from #1542 even adding import type { A } from 'a.js' test case.

@Hypnosphi
Copy link
Contributor

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

@nicolashenry
Copy link
Contributor

nicolashenry commented Sep 21, 2020

@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
Copy link
Contributor

@ljharb I created #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).

@ljharb ljharb closed this as completed in 5ade156 Oct 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment