diff --git a/.gitignore b/.gitignore index a01de720c..bb59bce59 100644 --- a/.gitignore +++ b/.gitignore @@ -49,3 +49,6 @@ lib/ yarn.lock package-lock.json npm-shrinkwrap.json + +# macOS +.DS_Store diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c5da1d45..1687ebee0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel - [`no-webpack-loader-syntax`]/TypeScript: avoid crash on missing name ([#1947], thanks @leonardodino) - [`no-extraneous-dependencies`]: Add package.json cache ([#1948], thanks @fa93hws) - [`prefer-default-export`]: handle empty array destructuring ([#1965], thanks @ljharb) +- [`no-unused-modules`]: make type imports mark a module as used (fixes #1924) ([#1974], thanks [@cherryblossom000]) ## [2.22.1] - 2020-09-27 ### Fixed @@ -739,6 +740,8 @@ for info on changes for earlier releases. [`memo-parser`]: ./memo-parser/README.md +[#1974]: https://github.com/benmosher/eslint-plugin-import/pull/1974 +[#1924]: https://github.com/benmosher/eslint-plugin-import/issues/1924 [#1965]: https://github.com/benmosher/eslint-plugin-import/issues/1965 [#1948]: https://github.com/benmosher/eslint-plugin-import/pull/1948 [#1947]: https://github.com/benmosher/eslint-plugin-import/pull/1947 @@ -1289,3 +1292,4 @@ for info on changes for earlier releases. [@tomprats]: https://github.com/tomprats [@straub]: https://github.com/straub [@andreubotella]: https://github.com/andreubotella +[@cherryblossom000]: https://github.com/cherryblossom000 diff --git a/src/ExportMap.js b/src/ExportMap.js index d75999542..9ffe7ac8d 100644 --- a/src/ExportMap.js +++ b/src/ExportMap.js @@ -83,8 +83,8 @@ export default class ExportMap { /** * ensure that imported name fully resolves. - * @param {[type]} name [description] - * @return {Boolean} [description] + * @param {string} name + * @return {{ found: boolean, path: ExportMap[] }} */ hasDeep(name) { if (this.namespace.has(name)) return { found: true, path: [this] }; @@ -241,8 +241,8 @@ const availableDocStyleParsers = { /** * parse JSDoc from leading comments - * @param {...[type]} comments [description] - * @return {{doc: object}} + * @param {object[]} comments + * @return {{ doc: object }} */ function captureJsDoc(comments) { let doc; @@ -286,6 +286,8 @@ function captureTomDoc(comments) { } } +const supportedImportTypes = new Set(['ImportDefaultSpecifier', 'ImportNamespaceSpecifier']); + ExportMap.get = function (source, context) { const path = resolve(source, context); if (path == null) return null; @@ -410,43 +412,27 @@ ExportMap.parse = function (path, content, context) { return object; } - function captureDependency(declaration) { - if (declaration.source == null) return null; - if (declaration.importKind === 'type') return null; // skip Flow type imports - const importedSpecifiers = new Set(); - const supportedTypes = new Set(['ImportDefaultSpecifier', 'ImportNamespaceSpecifier']); - let hasImportedType = false; - if (declaration.specifiers) { - declaration.specifiers.forEach(specifier => { - const isType = specifier.importKind === 'type'; - hasImportedType = hasImportedType || isType; - - if (supportedTypes.has(specifier.type) && !isType) { - importedSpecifiers.add(specifier.type); - } - if (specifier.type === 'ImportSpecifier' && !isType) { - importedSpecifiers.add(specifier.imported.name); - } - }); - } - - // only Flow types were imported - if (hasImportedType && importedSpecifiers.size === 0) return null; + function captureDependency({ source }, isOnlyImportingTypes, importedSpecifiers = new Set()) { + if (source == null) return null; - const p = remotePath(declaration.source.value); + const p = remotePath(source.value); if (p == null) return null; + + const declarationMetadata = { + // capturing actual node reference holds full AST in memory! + source: { value: source.value, loc: source.loc }, + isOnlyImportingTypes, + importedSpecifiers, + }; + const existing = m.imports.get(p); - if (existing != null) return existing.getter; + if (existing != null) { + existing.declarations.add(declarationMetadata); + return existing.getter; + } const getter = thunkFor(p, context); - m.imports.set(p, { - getter, - source: { // capturing actual node reference holds full AST in memory! - value: declaration.source.value, - loc: declaration.source.loc, - }, - importedSpecifiers, - }); + m.imports.set(p, { getter, declarations: new Set([declarationMetadata]) }); return getter; } @@ -483,14 +469,32 @@ ExportMap.parse = function (path, content, context) { } if (n.type === 'ExportAllDeclaration') { - const getter = captureDependency(n); + const getter = captureDependency(n, n.exportKind === 'type'); if (getter) m.dependencies.add(getter); return; } // capture namespaces in case of later export if (n.type === 'ImportDeclaration') { - captureDependency(n); + // import type { Foo } (TS and Flow) + const declarationIsType = n.importKind === 'type'; + let isOnlyImportingTypes = declarationIsType; + const importedSpecifiers = new Set(); + n.specifiers.forEach(specifier => { + if (supportedImportTypes.has(specifier.type)) { + importedSpecifiers.add(specifier.type); + } + if (specifier.type === 'ImportSpecifier') { + importedSpecifiers.add(specifier.imported.name); + } + + // import { type Foo } (Flow) + if (!declarationIsType) { + isOnlyImportingTypes = specifier.importKind === 'type'; + } + }); + captureDependency(n, isOnlyImportingTypes, importedSpecifiers); + const ns = n.specifiers.find(s => s.type === 'ImportNamespaceSpecifier'); if (ns) { namespaces.set(ns.local.name, n.source.value); diff --git a/src/rules/no-cycle.js b/src/rules/no-cycle.js index 21a428179..b72bd34fd 100644 --- a/src/rules/no-cycle.js +++ b/src/rules/no-cycle.js @@ -48,12 +48,19 @@ module.exports = { return; // ignore external modules } - const imported = Exports.get(sourceNode.value, context); - - if (importer.importKind === 'type') { - return; // no Flow import resolution + if ( + importer.type === 'ImportDeclaration' && ( + // import type { Foo } (TS and Flow) + importer.importKind === 'type' || + // import { type Foo } (Flow) + importer.specifiers.every(({ importKind }) => importKind === 'type') + ) + ) { + return; // ignore type imports } + const imported = Exports.get(sourceNode.value, context); + if (imported == null) { return; // no-unresolved territory } @@ -70,15 +77,20 @@ module.exports = { if (traversed.has(m.path)) return; traversed.add(m.path); - for (const [path, { getter, source }] of m.imports) { + for (const [path, { getter, declarations }] of m.imports) { if (path === myPath) return true; if (traversed.has(path)) continue; - if (ignoreModule(source.value)) continue; - if (route.length + 1 < maxDepth) { - untraversed.push({ - mget: getter, - route: route.concat(source), - }); + for (const { source, isOnlyImportingTypes } of declarations) { + if (ignoreModule(source.value)) continue; + // Ignore only type imports + if (isOnlyImportingTypes) continue; + + if (route.length + 1 < maxDepth) { + untraversed.push({ + mget: getter, + route: route.concat(source), + }); + } } } } diff --git a/src/rules/no-unused-modules.js b/src/rules/no-unused-modules.js index 16299ac56..dfea6de6d 100644 --- a/src/rules/no-unused-modules.js +++ b/src/rules/no-unused-modules.js @@ -114,7 +114,7 @@ const importList = new Map(); * keys are the paths to the modules containing the exports, while the * lower-level Map keys are the specific identifiers or special AST node names * being exported. The leaf-level metadata object at the moment only contains a - * `whereUsed` propoerty, which contains a Set of paths to modules that import + * `whereUsed` property, which contains a Set of paths to modules that import * the name. * * For example, if we have a file named bar.js containing the following exports: @@ -216,12 +216,10 @@ const prepareImportsAndExports = (srcFiles, context) => { if (isNodeModule(key)) { return; } - let localImport = imports.get(key); - if (typeof localImport !== 'undefined') { - localImport = new Set([...localImport, ...value.importedSpecifiers]); - } else { - localImport = value.importedSpecifiers; - } + const localImport = imports.get(key) || new Set(); + value.declarations.forEach(({ importedSpecifiers }) => + importedSpecifiers.forEach(specifier => localImport.add(specifier)) + ); imports.set(key, localImport); }); importList.set(file, imports); diff --git a/tests/files/no-unused-modules/typescript/file-ts-f-import-type.ts b/tests/files/no-unused-modules/typescript/file-ts-f-import-type.ts new file mode 100644 index 000000000..dd8204377 --- /dev/null +++ b/tests/files/no-unused-modules/typescript/file-ts-f-import-type.ts @@ -0,0 +1 @@ +import type {g} from './file-ts-g-used-as-type' diff --git a/tests/files/no-unused-modules/typescript/file-ts-f.ts b/tests/files/no-unused-modules/typescript/file-ts-f.ts new file mode 100644 index 000000000..f3a1ca7ab --- /dev/null +++ b/tests/files/no-unused-modules/typescript/file-ts-f.ts @@ -0,0 +1 @@ +import {g} from './file-ts-g'; diff --git a/tests/files/no-unused-modules/typescript/file-ts-g-used-as-type.ts b/tests/files/no-unused-modules/typescript/file-ts-g-used-as-type.ts new file mode 100644 index 000000000..fe5318fbe --- /dev/null +++ b/tests/files/no-unused-modules/typescript/file-ts-g-used-as-type.ts @@ -0,0 +1 @@ +export interface g {} diff --git a/tests/files/no-unused-modules/typescript/file-ts-g.ts b/tests/files/no-unused-modules/typescript/file-ts-g.ts new file mode 100644 index 000000000..fe5318fbe --- /dev/null +++ b/tests/files/no-unused-modules/typescript/file-ts-g.ts @@ -0,0 +1 @@ +export interface g {} diff --git a/tests/src/rules/no-unused-modules.js b/tests/src/rules/no-unused-modules.js index 0e7826a52..5906c9afb 100644 --- a/tests/src/rules/no-unused-modules.js +++ b/tests/src/rules/no-unused-modules.js @@ -827,6 +827,31 @@ context('TypeScript', function () { parser: parser, filename: testFilePath('./no-unused-modules/typescript/file-ts-e-used-as-type.ts'), }), + // Should also be valid when the exporting files are linted before the importing ones + test({ + options: unusedExportsTypescriptOptions, + code: `export interface g {}`, + parser, + filename: testFilePath('./no-unused-modules/typescript/file-ts-g.ts'), + }), + test({ + options: unusedExportsTypescriptOptions, + code: `import {g} from './file-ts-g';`, + parser, + filename: testFilePath('./no-unused-modules/typescript/file-ts-f.ts'), + }), + test({ + options: unusedExportsTypescriptOptions, + code: `export interface g {};`, + parser, + filename: testFilePath('./no-unused-modules/typescript/file-ts-g-used-as-type.ts'), + }), + test({ + options: unusedExportsTypescriptOptions, + code: `import type {g} from './file-ts-g';`, + parser, + filename: testFilePath('./no-unused-modules/typescript/file-ts-f-import-type.ts'), + }), ], invalid: [ test({ @@ -940,4 +965,3 @@ describe('ignore flow types', () => { invalid: [], }); }); -