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

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

Closed
maneetgoyal opened this issue Oct 12, 2020 · 8 comments · Fixed by #1974
Closed

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

maneetgoyal opened this issue Oct 12, 2020 · 8 comments · Fixed by #1974

Comments

@maneetgoyal
Copy link

  1. The use of import/no-unused-modules flags raises a false error: exported declaration 'XYZ' not used within other modules when using import type { XYZ } from "file.ts" notation. 🐛

  2. Source Code:

// file1.ts

export interface Foo {

}
// file2.ts

import type { Foo } from "./file1.ts";
  1. Current Behavior: Lint error
exported declaration 'Foo' not used within other modules eslint(import/no-unused-modules)
  1. Expected behavior: No error

Other notes:

I could use import { Foo } from "./file1.ts" but this coding style conflicts with the @typescript-eslint/consistent-type-imports rule and I get a lint error:

Import "NumberFormatProps" is only used as types eslint(@typescript-eslint/consistent-type-imports)

Will be really great if import/no-unused-modules can be upgraded to handle import type ... syntax. 🎉


Attn. #1819
cc @nicolashenry @ljharb

@ljharb
Copy link
Member

ljharb commented Oct 18, 2020

I agree, import type should be considered here.

@nicolashenry
Copy link
Contributor

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.

@ljharb
Copy link
Member

ljharb commented Oct 18, 2020

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.

@cherryblossom000
Copy link
Contributor

cherryblossom000 commented Jan 12, 2021

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)

@ljharb
Copy link
Member

ljharb commented Jan 12, 2021

@cherryblossom000 that sounds like a great idea. Want to make a PR?

@cherryblossom000
Copy link
Contributor

cherryblossom000 commented Jan 12, 2021

@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})
}

@ljharb
Copy link
Member

ljharb commented Jan 12, 2021

That also seems reasonable.

@cherryblossom000
Copy link
Contributor

cherryblossom000 commented Jan 13, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

4 participants