search

import-js / eslint-plugin-import

ESLint plugin with rules that help validate proper imports.
MIT License
5.57k stars 1.57k forks source link

no-cycle does not detect require()-based cycles #2702

Open srsudar opened 1 year ago

srsudar commented 1 year ago

Following up on #941, as requested here.

From my testing, the no-cycle rule DOES NOT DETECT circular dependencies if they are imported via require() calls.

This seems to be the intended behavior, however. As mentioned in this comment, I found this quote from the no-cycle docs:

By default, this rule only detects cycles for ES6 imports, but see the no-unresolved options as this rule also supports the same commonjs and amd flags. However, these flags only impact which import types are linted; the import/export infrastructure only registers import statements in dependencies, so cycles created by require within imported modules may not be detected.

However, a follow-up comment says that require() cycles are in fact supported, and suggested I open a new issue.

As far as I can tell, the original commenter's repo on that issue shows the problem quite well:

https://github.com/simonbuchan/eslint-import-cycle-example

Clone that, then:

$ npm install
$ npm run lint

> eslint-import-cycle@1.0.0 lint
> eslint .

/eslint-import-cycle-example/import-a.js
  1:1  error  Dependency cycle detected  import/no-cycle

/eslint-import-cycle-example/import-b.js
  1:1  error  Dependency cycle detected  import/no-cycle

✖ 2 problems (2 errors, 0 warnings)

It should also be detecting the cycle in require-a.js.

Am I misusing something?

srsudar commented 1 year ago

Do you have a guide somewhere on how to do local development with this plugin? I'd like to poke around a bit, but I haven't figured out how to point eslint at a locally installed repo. Do I have to just copy the working directory into node_modules?

ljharb commented 1 year ago

No, you'd use npm link.

srsudar commented 1 year ago

I have a working demo in #2706. You definitely wouldn't want to merge as-is, because I just hardcoded a return true in lieu of refining the regex there. I don't totally follow what it's doing, so I didn't try to fix it.

The output on the demo repo above, on that branch, is now:

eslint-import-cycle-example [main*?] $ npm run lint

> eslint-import-cycle@1.0.0 lint
> eslint .

/eslint-import-cycle-example/import-a.js
  1:1  error  Dependency cycle detected  import/no-cycle

/eslint-import-cycle-example/import-b.js
  1:1  error  Dependency cycle detected  import/no-cycle

/eslint-import-cycle-example/require-a.js
  1:11  error  Dependency cycle detected  import/no-cycle

/eslint-import-cycle-example/require-b.js
  1:11  error  Dependency cycle detected  import/no-cycle

✖ 4 problems (4 errors, 0 warnings)

And as an aside, for anybody else, I got this working pretty cleanly for local development via:

eslint-plugin-import $ npm run build
eslint-plugin-import $ npm link

eslint-import-cycle-example $ npm link eslint-plugin-import
eslint-import-cycle-example $ npm run lint