rule: no-cycle #1052
rule: no-cycle #1052
Conversation
(even though moduleVisitor handles it)
| ### Options | ||
|
|
||
| By default, this rule only detects cycles for ES6 imports, but see the [`no-unresolved` options](./no-unresolved.md#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 |
There was a problem hiding this comment.
It seems super important that this can detect requires as well as imports. What’s involved in making that happen?
There was a problem hiding this comment.
Yeah, I knew you'd pick up on that as I realized it and wrote it out. 😅
Right now, all the normal static analysis inspection of dependencies only picks up imports/exports from the topmost scope of the module.
Could also potentially nab any single-argument requires that are either alone or in AssignmentExpressions but everything gets dicier once requires are in the mix, since they could be present in any scope in the module, technically.
I want to avoid blocking merging and shipping this on supporting deep registration of CJS requires, though.
There was a problem hiding this comment.
Could you use an ast selector like CallExpression[callee.name=“require”] for the visitor?
There was a problem hiding this comment.
For sure. That's certainly what the moduleVisitor implementation does to find and lint requires.
It would be a substantial improvement, and I think I am probably overstating the complexity. I'll try to take a second look.
|
|
||
| export function bar() { | ||
| return "side effects???" | ||
| } |
There was a problem hiding this comment.
nbd ofc but missing a trailing newline
tests/src/rules/no-cycle.js
Outdated
| filename: testFilePath('./cycles/depth-zero.js'), | ||
| })) | ||
|
|
||
| describe.only("no-cycle", () => { |
There was a problem hiding this comment.
this only probably needs to be removed
Rule implementation for #941.
I need to do a final smoke test locally before merging, but wanted to open this up for implementation/docs/tests comments. Most notably, this first draft won't detect cycles introduced by deep
requires. It will only lintrequirecalls that import modules that import modules.