-
-
Notifications
You must be signed in to change notification settings - Fork 781
fix(noImportCycles): prevent flagging on single file import cycling #6765
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
fix(noImportCycles): prevent flagging on single file import cycling #6765
Conversation
🦋 Changeset detectedLatest commit: 92d3e18 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
CodSpeed Performance ReportMerging #6765 will not alter performanceComparing Summary
Footnotes
|
52ec1a7 to
eb47749
Compare
arendjr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Do you think it makes sense to restrict this exception to exports only? It would be sufficient for the use case from #6569, so I wonder if there could ever be a reason why you'd want to import from the same file?
Co-authored-by: Arend van Beelen jr. <[email protected]>
|
I will look into it, thank you for the review |
|
@emilyinure any updates? |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
.changeset/icy-results-wonder.md (1)
5-19: Example doesn't demonstrate self-referential export.The changeset description says "Allow files to export from themselves", but the example shows exporting from
'./test'while the file isexample.js. This doesn't demonstrate the self-reference scenario that the fix addresses.As suggested in the past review comment, consider updating the example to show an actual self-reference:
**example.js** ```js export function example1() { return 1; } export function example2() { return 2; } -export * as Example from './test'; +export * as Example from './example';Alternatively, if the file should export from itself, use: ```js export * as Example from './example.js';This would clearly demonstrate the fix for self-referential exports.
Based on learnings
🧹 Nitpick comments (1)
crates/biome_js_analyze/src/lint/nursery/no_import_cycles.rs (1)
173-177: Guard logic correctly prevents false positives for self-references.The implementation properly identifies single-file cycles by checking if the stack is empty (no intermediate files) and the start path matches the resolved path. This aligns with the PR objective to fix issue #6569.
Minor suggestion for clarity: since at this point
resolved_path == ctx.file_path()(from line 172), you could compare paths directly:-if stack.is_empty() && start_path.as_str() == resolved_path { +if stack.is_empty() && start_path == resolved_path {or even more explicitly:
-if stack.is_empty() && start_path.as_str() == resolved_path { +if stack.is_empty() && start_path == ctx.file_path() {This avoids the string conversion and makes the intent clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/biome_js_analyze/tests/specs/nursery/noImportCycles/valid.js.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
.changeset/icy-results-wonder.md(1 hunks)crates/biome_js_analyze/src/lint/nursery/no_import_cycles.rs(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noImportCycles/valid.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: arendjr
Repo: biomejs/biome PR: 7593
File: crates/biome_service/src/workspace/server.rs:1306-1306
Timestamp: 2025-09-25T12:32:59.003Z
Learning: In the biomejs/biome project, do not flag compilation errors during code review as they are handled by the existing test infrastructure and CI. Focus on other code quality aspects instead.
🧬 Code graph analysis (1)
crates/biome_js_analyze/tests/specs/nursery/noImportCycles/valid.js (1)
crates/biome_js_analyze/tests/specs/nursery/noImportCycles/invalidFoobar.js (1)
bar(7-9)
🪛 markdownlint-cli2 (0.18.1)
.changeset/icy-results-wonder.md
9-9: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (2)
crates/biome_js_analyze/src/lint/nursery/no_import_cycles.rs (1)
180-186: Type annotation improves code clarity.The explicit type annotation for the
pathscollection enhances readability without changing behavior.crates/biome_js_analyze/tests/specs/nursery/noImportCycles/valid.js (1)
7-15: The review comment is based on a misunderstanding of the test's purpose.This is a test file for the
noImportCycleslinter rule specifically, not a validation of general JavaScript correctness. The test file is named "valid.js" because it should not trigger the noImportCycles rule—and it doesn't. The snap file confirms the test expects zero diagnostics.The rule only checks for circular imports. The three concerns raised (name collision, missing export, mismatched structure) are semantic/syntax issues that:
- Fall outside the scope of the noImportCycles rule
- Would be caught by different linting rules if needed
- Are irrelevant to whether
valid.jsparticipates in an import cycleThe file correctly imports from
./invalidFoobar, which has a cycle (invalidFoobar ↔ invalidBaz), but that cycle does not lead back tovalid.js, so the rule correctly produces no diagnostics.Likely an incorrect or invalid review comment.
dyc3
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
…iomejs#6765) Co-authored-by: Arend van Beelen jr. <[email protected]> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Emanuele Stoppa <[email protected]>
Summary
Allows cases where a cycling import is contained to a single file.
fixes #6569
Test Plan
Snap tests in /specs/nursery/noImportCycles/valid.js
Passing, along with existing test cases passing.