Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
srsudar opened this issue Feb 2, 2023 · 3 comments
Open

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

srsudar opened this issue Feb 2, 2023 · 3 comments

Comments

@srsudar
Copy link

srsudar commented Feb 2, 2023

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
Copy link
Author

srsudar commented Feb 3, 2023

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
Copy link
Member

ljharb commented Feb 3, 2023

No, you'd use npm link.

@srsudar
Copy link
Author

srsudar commented Feb 4, 2023

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

@ljharb ljharb removed the question label Apr 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants