Fix a couple of no-cycle bugs

CHANGELOG.md

@@ -8,6 +8,8 @@ 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])
@@ -790,6 +792,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

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) {

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

tests/files/cycles/flow-types-only-importing-multiple-types.js

@@ -0,0 +1,3 @@
+// @flow
+
+import { type FooType, type BarType } from './depth-zero';

tests/files/cycles/flow-types-only-importing-type.js

@@ -0,0 +1,3 @@
+// @flow
+
+import type { FooType } from './depth-zero';

tests/files/cycles/flow-types-some-type-imports.js

@@ -0,0 +1,3 @@
+// @flow
+
+import { foo, type BarType } from './depth-zero'

tests/src/rules/no-cycle.js

@@ -73,12 +73,25 @@ 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({
       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.`)],