[import/no-unused-modules] False positive while importing TS interfaces #1924

Activity

1. ljharb

   added

   typescript

   on Oct 18, 2020
2. ljharb commented on Oct 18, 2020

   

   ljharb

   on Oct 18, 2020

   Member

   More actions

   I agree, import type should be considered here.
3. ljharb

   added

   help wanted

   on Oct 18, 2020
4. nicolashenry commented on Oct 18, 2020

   

   nicolashenry

   on Oct 18, 2020

   More actions

   I think it is probably working under some conditions depending on loading order like Flow type imports but I still don't know why type import loading order is different than regular ones. I personally use regular imports to import interfaces and other types so I don't have this issue.
5. ljharb commented on Oct 18, 2020

   

   ljharb

   on Oct 18, 2020

   Member

   More actions

   It’s a best practice to only ever use import type for type values, so anything we can do to make that easier, we should do.

   👍React with 👍5Reacted by Nicolas HENRY, Manufac, Vitaliy, Patrick Braune and Cheeshian Chuah
6. cherryblossom000 commented on Jan 12, 2021

   

   cherryblossom000

   on Jan 12, 2021

   Last edited by cherryblossom000

   Contributor

   More actions

   I had a quick look at the code and it looks like type imports are ignored because of this line in captureDependency:

   https://github.com/benmosher/eslint-plugin-import/blob/ff50964df1234515bc6f7969b90d87ada1982008/src/ExportMap.js#L414

   This was introduced in #1494 so that no-cycle would ignore type imports.

   Maybe a parameter ignoreTypeImports should be added to ExportMap.parse (and also ExportMap.for and ExportMap.get) so no-cycle can ignore type imports but no-unused-modules doesn't. This won't work, see #1924 (comment)
7. ljharb commented on Jan 12, 2021

   

   ljharb

   on Jan 12, 2021

   Member

   More actions

   @cherryblossom000 that sounds like a great idea. Want to make a PR?
8. cherryblossom000 commented on Jan 12, 2021

   

   cherryblossom000

   on Jan 12, 2021

   Last edited by cherryblossom000

   Contributor

   More actions

   @ljharb I'm working on it right now. However, I realised the solution I proposed above wouldn't work as the export maps are cached. I'm thinking of having a isType property for the import specifiers something like this:

   if (supportedTypes.has(specifier.type)) {
     importedSpecifiers.add({name: specifier.type, isType})
   }
   if (specifier.type === 'ImportSpecifier') {
     importedSpecifiers.add({name: specifier.imported.name, isType})
   }
9. ljharb commented on Jan 12, 2021

   

   ljharb

   on Jan 12, 2021

   Member

   More actions

   That also seems reasonable.
10. cherryblossom000 commented on Jan 13, 2021

    

    cherryblossom000

    on Jan 13, 2021

    Last edited by cherryblossom000

    Contributor

    More actions

    This is a bit more complicated than I expected. All these tests:

    tests

    https://github.com/benmosher/eslint-plugin-import/blob/ff50964df1234515bc6f7969b90d87ada1982008/tests/src/rules/no-unused-modules.js#L806-L829

    fail if this test is removed or moved to after the other tests:

    other test

    https://github.com/benmosher/eslint-plugin-import/blob/ff50964df1234515bc6f7969b90d87ada1982008/tests/src/rules/no-unused-modules.js#L790-L805

    In other words, the tests for files that are only imported as types only succeed when the file importing them is tested first.

    I'm quite busy at the moment but I'll try to get around to why this happens as soon as I can.

    ---

    This seems to be because of these lines in updateImportUsage:

    https://github.com/benmosher/eslint-plugin-import/blob/ff50964df1234515bc6f7969b90d87ada1982008/src/rules/no-unused-modules.js#L701-L707

    newImports is then used to update whereUsed for that file:

    https://github.com/benmosher/eslint-plugin-import/blob/ff50964df1234515bc6f7969b90d87ada1982008/src/rules/no-unused-modules.js#L858-L864

    For example, if we had these files:

    a.ts

    export interface A {}

    b.ts

    import type {A} from './a'

    If b.ts is linted first, the Program:exit selector runs, which calls updateImportUsage. updateImportUsage iterates through the program's statements, and if it finds an ImportDeclaration, it adds all the ImportSpecifiers to newImports (newImports.set('A', 'a.ts')). All the newImports that aren't in the imports parsed by ExportMap are added to whereUsed, so a.ts's export map would be Map(1) { 'A' => { whereUsed: Set(1) { 'b.ts' } } } When a.ts is linted, the rule sees that A has been used in b.ts, and so doesn't report an error.

    However, if a.ts is linted first, the export map of a.ts would be Map(1) { 'A' => { whereUsed: Set(0) { } } } and reports the error.

    > npx eslint a.ts b.ts
    
    a.ts
      1:1  error  exported declaration 'A' not used within other modules  import/no-unused-modules
    
    ✖ 1 problem (1 error, 0 warnings)
    
    > npx eslint b.ts a.ts # no output
11. ljharb

    closed this as completedin 462b016on Jan 18, 2021