[New] no-cycle: add option to allow cycle via dynamic import

Author: GerkinDev

Diff for: CHANGELOG.md

@@ -24,6 +24,7 @@ This change log adheres to standards from [Keep a CHANGELOG](https://keepachange
 - [`no-named-default`, `no-default-export`, `prefer-default-export`, `no-named-export`, `export`, `named`, `namespace`, `no-unused-modules`]: support arbitrary module namespace names ([#2358], thanks [@sosukesuzuki])
 - [`no-dynamic-require`]: support dynamic import with espree ([#2371], thanks [@sosukesuzuki])
 - [`no-relative-packages`]: add fixer ([#2381], thanks [@forivall])
+- [`no-cycle`]: add `allowUnsafeDynamicCyclicDependency` option ([#2387], thanks [@GerkinDev])
 
 ### Fixed
 - [`default`]: `typescript-eslint-parser`: avoid a crash on exporting as namespace (thanks [@ljharb])

Diff for: docs/rules/no-cycle.md

@@ -75,6 +75,22 @@ import { a } from './dep-a.js' // not reported as this module is external
 
 Its value is `false` by default, but can be set to `true` for reducing total project lint time, if needed.
 
+#### `allowUnsafeDynamicCyclicDependency`
+
+This option disable reporting of errors if a cycle is detected with at least one dynamic import.
+
+```js
+// bar.js
+import { foo } from './foo';
+export const bar = foo;
+
+// foo.js
+export const foo = 'Foo';
+export function getBar() { return import('./bar'); }
+```
+
+> Cyclic dependency are **always** a dangerous anti-pattern as discussed extensively in [#2265](https://github.com/import-js/eslint-plugin-import/issues/2265). Please be extra careful about using this option.
+
 ## When Not To Use It
 
 This rule is comparatively computationally expensive. If you are pressed for lint

Diff for: src/ExportMap.js

@@ -395,6 +395,7 @@ ExportMap.parse = function (path, content, context) {
           loc: source.loc,
         },
         importedSpecifiers,
+        dynamic: true,
       }]),
     });
   }

Diff for: src/rules/no-cycle.js

@@ -33,6 +33,11 @@ module.exports = {
         type: 'boolean',
         default: false,
       },
+      allowUnsafeDynamicCyclicDependency: {
+        description: 'Allow cyclic dependency if there is at least one dynamic import in the chain',
+        type: 'boolean',
+        default: false,
+      },
     })],
   },
 
@@ -52,6 +57,13 @@ module.exports = {
       if (ignoreModule(sourceNode.value)) {
         return; // ignore external modules
       }
+      if (options.allowUnsafeDynamicCyclicDependency && (
+        // Ignore `import()`
+        importer.type === 'ImportExpression' ||
+        // `require()` calls are always checked (if possible)
+        (importer.type === 'CallExpression' && importer.callee.name !== 'require'))) {
+        return; // cycle via dynamic import allowed by config
+      }
 
       if (
         importer.type === 'ImportDeclaration' && (
@@ -89,6 +101,12 @@ module.exports = {
             // Ignore only type imports
             !isOnlyImportingTypes,
           );
+          
+          /*
+          If cyclic dependency is allowed via dynamic import, skip checking if any module is imported dynamically
+          */
+          if (options.allowUnsafeDynamicCyclicDependency && toTraverse.some(d => d.dynamic)) return;
+
           /*
           Only report as a cycle if there are any import declarations that are considered by
           the rule. For example:

Diff for: tests/files/cycles/es6/depth-one-dynamic.js

@@ -0,0 +1 @@
+export const bar = () => import("../depth-zero").then(({foo}) => foo);

Diff for: tests/src/rules/no-cycle.js

@@ -1,4 +1,4 @@
-import { parsers, test as _test, testFilePath } from '../utils';
+import { parsers, test as _test, testFilePath, testVersion as _testVersion } from '../utils';
 
 import { RuleTester } from 'eslint';
 import flatMap from 'array.prototype.flatmap';
@@ -11,6 +11,9 @@ const error = message => ({ message });
 const test = def => _test(Object.assign(def, {
   filename: testFilePath('./cycles/depth-zero.js'),
 }));
+const testVersion = (specifier, t) => _testVersion(specifier, () => Object.assign(t(), {
+  filename: testFilePath('./cycles/depth-zero.js'),
+}));
 
 const testDialects = ['es6'];
 
@@ -73,7 +76,28 @@ ruleTester.run('no-cycle', rule, {
         code: `import type { FooType, BarType } from "./${testDialect}/depth-one"`,
         parser: parsers.BABEL_OLD,
       }),
-    ]),
+      test({
+        code: `function bar(){ return import("./${testDialect}/depth-one"); } // #2265 1`,
+        options: [{ allowUnsafeDynamicCyclicDependency: true }],
+        parser: parsers.BABEL_OLD,
+      }),
+      test({
+        code: `import { foo } from "./${testDialect}/depth-one-dynamic"; // #2265 2`,
+        options: [{ allowUnsafeDynamicCyclicDependency: true }],
+        parser: parsers.BABEL_OLD,
+      }),
+    ].concat(parsers.TS_NEW ? [
+      test({
+        code: `function bar(){ return import("./${testDialect}/depth-one"); } // #2265 3`,
+        options: [{ allowUnsafeDynamicCyclicDependency: true }],
+        parser: parsers.TS_NEW,
+      }),
+      test({
+        code: `import { foo } from "./${testDialect}/depth-one-dynamic"; // #2265 4`,
+        options: [{ allowUnsafeDynamicCyclicDependency: true }],
+        parser: parsers.TS_NEW,
+      }),
+    ] : [])),
 
     test({
       code: 'import { bar } from "./flow-types"',
@@ -112,62 +136,83 @@ ruleTester.run('no-cycle', rule, {
       },
     }),
 
-    flatMap(testDialects, (testDialect) => [
+    // Ensure behavior does not change for those tests, with or without `
+    flatMap(testDialects, (testDialect) => flatMap([
+      {},
+      { allowUnsafeDynamicCyclicDependency: true },
+    ], (opts) => [
       test({
         code: `import { foo } from "./${testDialect}/depth-one"`,
+        options: [{ ...opts }],
         errors: [error(`Dependency cycle detected.`)],
       }),
       test({
         code: `import { foo } from "./${testDialect}/depth-one"`,
-        options: [{ maxDepth: 1 }],
+        options: [{ ...opts, maxDepth: 1 }],
         errors: [error(`Dependency cycle detected.`)],
       }),
       test({
         code: `const { foo } = require("./${testDialect}/depth-one")`,
         errors: [error(`Dependency cycle detected.`)],
-        options: [{ commonjs: true }],
+        options: [{ ...opts, commonjs: true }],
       }),
       test({
         code: `require(["./${testDialect}/depth-one"], d1 => {})`,
         errors: [error(`Dependency cycle detected.`)],
-        options: [{ amd: true }],
+        options: [{ ...opts, amd: true }],
       }),
       test({
         code: `define(["./${testDialect}/depth-one"], d1 => {})`,
         errors: [error(`Dependency cycle detected.`)],
-        options: [{ amd: true }],
+        options: [{ ...opts, amd: true }],
       }),
       test({
         code: `import { foo } from "./${testDialect}/depth-two"`,
+        options: [{ ...opts }],
         errors: [error(`Dependency cycle via ./depth-one:1`)],
       }),
       test({
         code: `import { foo } from "./${testDialect}/depth-two"`,
-        options: [{ maxDepth: 2 }],
+        options: [{ ...opts, maxDepth: 2 }],
         errors: [error(`Dependency cycle via ./depth-one:1`)],
       }),
       test({
         code: `const { foo } = require("./${testDialect}/depth-two")`,
         errors: [error(`Dependency cycle via ./depth-one:1`)],
-        options: [{ commonjs: true }],
+        options: [{ ...opts, commonjs: true }],
       }),
       test({
         code: `import { two } from "./${testDialect}/depth-three-star"`,
+        options: [{ ...opts }],
         errors: [error(`Dependency cycle via ./depth-two:1=>./depth-one:1`)],
       }),
       test({
         code: `import one, { two, three } from "./${testDialect}/depth-three-star"`,
+        options: [{ ...opts }],
         errors: [error(`Dependency cycle via ./depth-two:1=>./depth-one:1`)],
       }),
       test({
         code: `import { bar } from "./${testDialect}/depth-three-indirect"`,
+        options: [{ ...opts }],
         errors: [error(`Dependency cycle via ./depth-two:1=>./depth-one:1`)],
       }),
       test({
         code: `import { bar } from "./${testDialect}/depth-three-indirect"`,
+        options: [{ ...opts }],
         errors: [error(`Dependency cycle via ./depth-two:1=>./depth-one:1`)],
         parser: parsers.BABEL_OLD,
       }),
+      test({
+        code: `import { foo } from "./${testDialect}/depth-two"`,
+        options: [{ ...opts, maxDepth: Infinity }],
+        errors: [error(`Dependency cycle via ./depth-one:1`)],
+      }),
+      test({
+        code: `import { foo } from "./${testDialect}/depth-two"`,
+        options: [{ ...opts, maxDepth: '∞' }],
+        errors: [error(`Dependency cycle via ./depth-one:1`)],
+      }),
+    ]).concat([
       test({
         code: `import("./${testDialect}/depth-three-star")`,
         errors: [error(`Dependency cycle via ./depth-two:1=>./depth-one:1`)],
@@ -188,7 +233,29 @@ ruleTester.run('no-cycle', rule, {
         options: [{ maxDepth: '∞' }],
         errors: [error(`Dependency cycle via ./depth-one:1`)],
       }),
-    ]),
+      test({
+        code: `function bar(){ return import("./${testDialect}/depth-one"); } // #2265 5`,
+        errors: [error(`Dependency cycle detected.`)],
+        parser: parsers.BABEL_OLD,
+      }),
+    ]).concat(
+      testVersion('> 3', () => ({ // Dynamic import is not properly caracterized with eslint < 4
+        code: `import { foo } from "./${testDialect}/depth-one-dynamic"; // #2265 6`,
+        errors: [error(`Dependency cycle detected.`)],
+        parser: parsers.BABEL_OLD,
+      })),
+    ).concat(parsers.TS_NEW ? [
+      test({
+        code: `function bar(){ return import("./${testDialect}/depth-one"); } // #2265 7`,
+        errors: [error(`Dependency cycle detected.`)],
+        parser: parsers.TS_NEW,
+      }),
+      test({
+        code: `import { foo } from "./${testDialect}/depth-one-dynamic"; // #2265 8`,
+        errors: [error(`Dependency cycle detected.`)],
+        parser: parsers.TS_NEW,
+      }),
+    ] : [])),
 
     test({
       code: 'import { bar } from "./flow-types-depth-one"',