From b2f6ac8eedac22a241f9d295bcdd4eef4e1c85cf Mon Sep 17 00:00:00 2001 From: GerkinDev Date: Mon, 21 Feb 2022 14:00:51 +0100 Subject: [PATCH] [New] `no-cycle`: add option to allow cycle via dynamic import --- CHANGELOG.md | 1 + docs/rules/no-cycle.md | 16 ++++ src/ExportMap.js | 1 + src/rules/no-cycle.js | 18 +++++ tests/files/cycles/es6/depth-one-dynamic.js | 1 + tests/src/rules/no-cycle.js | 87 ++++++++++++++++++--- 6 files changed, 114 insertions(+), 10 deletions(-) create mode 100644 tests/files/cycles/es6/depth-one-dynamic.js diff --git a/CHANGELOG.md b/CHANGELOG.md index cb455b425..072911de6 100644 --- a/CHANGELOG.md +++ b/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 --git a/docs/rules/no-cycle.md b/docs/rules/no-cycle.md index 7f1b4255a..70b2ceb9b 100644 --- a/docs/rules/no-cycle.md +++ b/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 --git a/src/ExportMap.js b/src/ExportMap.js index d75c7ecd4..e18797a4d 100644 --- a/src/ExportMap.js +++ b/src/ExportMap.js @@ -395,6 +395,7 @@ ExportMap.parse = function (path, content, context) { loc: source.loc, }, importedSpecifiers, + dynamic: true, }]), }); } diff --git a/src/rules/no-cycle.js b/src/rules/no-cycle.js index e61c3be26..0aa362682 100644 --- a/src/rules/no-cycle.js +++ b/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 --git a/tests/files/cycles/es6/depth-one-dynamic.js b/tests/files/cycles/es6/depth-one-dynamic.js new file mode 100644 index 000000000..32dd3db4e --- /dev/null +++ b/tests/files/cycles/es6/depth-one-dynamic.js @@ -0,0 +1 @@ +export const bar = () => import("../depth-zero").then(({foo}) => foo); diff --git a/tests/src/rules/no-cycle.js b/tests/src/rules/no-cycle.js index 22e097dd2..ad29292c2 100644 --- a/tests/src/rules/no-cycle.js +++ b/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"',