From 30bba6a25e6d8ba6ae37b46072be2e232140402d Mon Sep 17 00:00:00 2001 From: cherryblossom <31467609+cherryblossom000@users.noreply.github.com> Date: Mon, 17 May 2021 19:28:04 +1000 Subject: [PATCH 1/2] [Fix] `no-cycle`: ignore imports where imported file only imports types of importing file This fixes this situation: `a.ts`: ```ts import { foo } from './b' ``` `b.ts`: ```ts import type { Bar } from './a' ``` Previously, `no-cycle` would have incorrectly reported a dependency cycle for the import in `a.ts`, even though `b.ts` is only importing types from `a.ts`. --- CHANGELOG.md | 2 ++ src/rules/no-cycle.js | 26 ++++++++++++------- ...low-types-only-importing-multiple-types.js | 3 +++ .../cycles/flow-types-only-importing-type.js | 3 +++ tests/src/rules/no-cycle.js | 8 ++++++ 5 files changed, 33 insertions(+), 9 deletions(-) create mode 100644 tests/files/cycles/flow-types-only-importing-multiple-types.js create mode 100644 tests/files/cycles/flow-types-only-importing-type.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 7397bc727..712bf21a2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel ### Fixed - [`no-restricted-paths`]: fix false positive matches ([#2090], thanks [@malykhinvi]) +- [`no-cycle`]: ignore imports where imported file only imports types of importing file ([#2083], thanks [@cherryblossom000]) ### Changed - [Docs] Add `no-relative-packages` to list of to the list of rules ([#2075], thanks [@arvigeus]) @@ -790,6 +791,7 @@ for info on changes for earlier releases. [`memo-parser`]: ./memo-parser/README.md [#2090]: https://github.com/benmosher/eslint-plugin-import/pull/2090 +[#2083]: https://github.com/benmosher/eslint-plugin-import/pull/2083 [#2075]: https://github.com/benmosher/eslint-plugin-import/pull/2075 [#2071]: https://github.com/benmosher/eslint-plugin-import/pull/2071 [#2034]: https://github.com/benmosher/eslint-plugin-import/pull/2034 diff --git a/src/rules/no-cycle.js b/src/rules/no-cycle.js index 74b77cbc3..9d9a28cd6 100644 --- a/src/rules/no-cycle.js +++ b/src/rules/no-cycle.js @@ -84,18 +84,26 @@ module.exports = { traversed.add(m.path); for (const [path, { getter, declarations }] of m.imports) { - if (path === myPath) return true; if (traversed.has(path)) continue; - for (const { source, isOnlyImportingTypes } of declarations) { - if (ignoreModule(source.value)) continue; + const toTraverse = [...declarations].filter(({ source, isOnlyImportingTypes }) => + !ignoreModule(source.value) && // Ignore only type imports - if (isOnlyImportingTypes) continue; + !isOnlyImportingTypes + ); + /* + Only report as a cycle if there are any import declarations that are considered by + the rule. For example: - if (route.length + 1 < maxDepth) { - untraversed.push({ - mget: getter, - route: route.concat(source), - }); + a.ts: + import { foo } from './b' // should not be reported as a cycle + + b.ts: + import type { Bar } from './a' + */ + if (path === myPath && toTraverse.length > 0) return true; + if (route.length + 1 < maxDepth) { + for (const { source } of toTraverse) { + untraversed.push({ mget: getter, route: route.concat(source) }); } } } diff --git a/tests/files/cycles/flow-types-only-importing-multiple-types.js b/tests/files/cycles/flow-types-only-importing-multiple-types.js new file mode 100644 index 000000000..ab61606fd --- /dev/null +++ b/tests/files/cycles/flow-types-only-importing-multiple-types.js @@ -0,0 +1,3 @@ +// @flow + +import { type FooType, type BarType } from './depth-zero'; diff --git a/tests/files/cycles/flow-types-only-importing-type.js b/tests/files/cycles/flow-types-only-importing-type.js new file mode 100644 index 000000000..b407da987 --- /dev/null +++ b/tests/files/cycles/flow-types-only-importing-type.js @@ -0,0 +1,3 @@ +// @flow + +import type { FooType } from './depth-zero'; diff --git a/tests/src/rules/no-cycle.js b/tests/src/rules/no-cycle.js index 965fa36b7..9620eac44 100644 --- a/tests/src/rules/no-cycle.js +++ b/tests/src/rules/no-cycle.js @@ -73,6 +73,14 @@ ruleTester.run('no-cycle', rule, { code: 'import { bar } from "./flow-types"', parser: require.resolve('babel-eslint'), }), + test({ + code: 'import { bar } from "./flow-types-only-importing-type"', + parser: require.resolve('babel-eslint'), + }), + test({ + code: 'import { bar } from "./flow-types-only-importing-multiple-types"', + parser: require.resolve('babel-eslint'), + }), ], invalid: [ test({ From 72b9c3da5d30c39a4dcb677c7a46d2ddae8aca7e Mon Sep 17 00:00:00 2001 From: cherryblossom <31467609+cherryblossom000@users.noreply.github.com> Date: Mon, 17 May 2021 19:23:39 +1000 Subject: [PATCH 2/2] [Fix] `no-cycle`: fix false negative when file imports a type after importing a value in Flow This fixes this situation: `a.js`: ```js import { foo } from './b' ``` `b.js`: ```js // @flow import { bar, type Baz } from './a' ``` Previously, `no-cycle` would have not reported a dependency cycle for the import in `a.js`, even though `b.js` is importing `bar`, which is not a type import, from `a.js`. This commit fixes that. --- CHANGELOG.md | 1 + src/ExportMap.js | 11 ++++++----- tests/files/cycles/flow-types-some-type-imports.js | 3 +++ tests/src/rules/no-cycle.js | 5 +++++ 4 files changed, 15 insertions(+), 5 deletions(-) create mode 100644 tests/files/cycles/flow-types-some-type-imports.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 712bf21a2..03526c700 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel ### Fixed - [`no-restricted-paths`]: fix false positive matches ([#2090], thanks [@malykhinvi]) - [`no-cycle`]: ignore imports where imported file only imports types of importing file ([#2083], thanks [@cherryblossom000]) +- [`no-cycle`]: fix false negative when file imports a type after importing a value in Flow ([#2083], thanks [@cherryblossom000]) ### Changed - [Docs] Add `no-relative-packages` to list of to the list of rules ([#2075], thanks [@arvigeus]) diff --git a/src/ExportMap.js b/src/ExportMap.js index 9cc7a089e..76b07f9dc 100644 --- a/src/ExportMap.js +++ b/src/ExportMap.js @@ -495,7 +495,9 @@ ExportMap.parse = function (path, content, context) { if (n.type === 'ImportDeclaration') { // import type { Foo } (TS and Flow) const declarationIsType = n.importKind === 'type'; - let isOnlyImportingTypes = declarationIsType; + // import './foo' or import {} from './foo' (both 0 specifiers) is a side effect and + // shouldn't be considered to be just importing types + let specifiersOnlyImportingTypes = n.specifiers.length; const importedSpecifiers = new Set(); n.specifiers.forEach(specifier => { if (supportedImportTypes.has(specifier.type)) { @@ -506,11 +508,10 @@ ExportMap.parse = function (path, content, context) { } // import { type Foo } (Flow) - if (!declarationIsType) { - isOnlyImportingTypes = specifier.importKind === 'type'; - } + specifiersOnlyImportingTypes = + specifiersOnlyImportingTypes && specifier.importKind === 'type'; }); - captureDependency(n, isOnlyImportingTypes, importedSpecifiers); + captureDependency(n, declarationIsType || specifiersOnlyImportingTypes, importedSpecifiers); const ns = n.specifiers.find(s => s.type === 'ImportNamespaceSpecifier'); if (ns) { diff --git a/tests/files/cycles/flow-types-some-type-imports.js b/tests/files/cycles/flow-types-some-type-imports.js new file mode 100644 index 000000000..9008ba1af --- /dev/null +++ b/tests/files/cycles/flow-types-some-type-imports.js @@ -0,0 +1,3 @@ +// @flow + +import { foo, type BarType } from './depth-zero' diff --git a/tests/src/rules/no-cycle.js b/tests/src/rules/no-cycle.js index 9620eac44..302db8351 100644 --- a/tests/src/rules/no-cycle.js +++ b/tests/src/rules/no-cycle.js @@ -87,6 +87,11 @@ ruleTester.run('no-cycle', rule, { code: 'import { foo } from "./depth-one"', errors: [error(`Dependency cycle detected.`)], }), + test({ + code: 'import { bar } from "./flow-types-some-type-imports"', + parser: require.resolve('babel-eslint'), + errors: [error(`Dependency cycle detected.`)], + }), test({ code: 'import { foo } from "cycles/external/depth-one"', errors: [error(`Dependency cycle detected.`)],