fix(linter): fix useImportExtensions handling of index files
#7209
Conversation
🦋 Changeset detectedLatest commit: 69058c5 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 |
WalkthroughAdded detection for import paths that end with a bare "index" (import_path_ends_with_index) and adjusted the rewrite logic so index resolution only rewrites to "index.{extension}" when the original path did not already explicitly end with "index". Paths that explicitly end with "index" are preserved (with any extension adjustments handled in the alternate branch). A test import "./sub/index" was added to invalid.ts to cover the case, and a changeset was included to patch @biomejs/biome for this behaviour correction. Possibly related issues
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (25)
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/biome_js_analyze/tests/specs/correctness/useImportExtensions/invalid.ts (1)
3-3: Great addition; please also cover dynamic and CJS variants.To fully exercise the fix, add:
- import('./sub/index')
- require('./sub/index')
These mirror the existing dynamic cases for './sub/foo' and ensure consistent behaviour across import forms.
crates/biome_js_analyze/src/lint/correctness/use_import_extensions.rs (1)
256-276: Simplify and harden index-path reconstruction.The current split/next_back/fold approach is a bit brittle. Trimming known suffixes and appending once is simpler and avoids accidental double slashes.
Apply this diff to simplify the construction:
- let mut path_parts = path.as_str().split('/'); - - // Remove trailing slash and useless path segment. - if module_path.ends_with('/') || module_path.ends_with("/.") { - path_parts.next_back(); - } - if module_path.ends_with("/./") { - path_parts.next_back(); - } - - let mut new_path = path_parts.fold(String::new(), |mut result, part| { - result.push_str(part); - result.push('/'); - - result - }); - - new_path.push_str("index."); - new_path.push_str(extension); - new_path + // Remove trailing slash and trailing `/.` or `/./`, then append `/index.{ext}` + let base = path + .as_str() + .trim_end_matches('/') + .trim_end_matches("/.") + .trim_end_matches("/./"); + let mut new_path = String::from(base); + if !new_path.ends_with('/') { + new_path.push('/'); + } + new_path.push_str("index."); + new_path.push_str(extension); + new_pathThis keeps behaviour identical for
./sub,./sub/,./sub/., and./sub/./, but is easier to reason about.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/biome_js_analyze/tests/specs/correctness/useImportExtensions/invalid.ts.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (2)
crates/biome_js_analyze/src/lint/correctness/use_import_extensions.rs(1 hunks)crates/biome_js_analyze/tests/specs/correctness/useImportExtensions/invalid.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
crates/biome_*/**/*
📄 CodeRabbit Inference Engine (CLAUDE.md)
Core crates must be located in
/crates/biome_*/
Files:
crates/biome_js_analyze/tests/specs/correctness/useImportExtensions/invalid.tscrates/biome_js_analyze/src/lint/correctness/use_import_extensions.rs
**/*.{rs,toml}
📄 CodeRabbit Inference Engine (CLAUDE.md)
Format code (Rust + TOML) using
just formatFormat Rust and TOML files before committing (use
just f/just format).
Files:
crates/biome_js_analyze/src/lint/correctness/use_import_extensions.rs
🔇 Additional comments (1)
crates/biome_js_analyze/src/lint/correctness/use_import_extensions.rs (1)
254-257: Correct guard to avoid duplicating “index”.The
import_path_ends_with_indexcheck paired withis_index_filecleanly prevents rewriting./sub/indexinto./sub/index/index.{ext}. This aligns with the rule’s intent while preserving the existing behaviour for directory-like imports (e.g../sub/).If you want to be doubly sure we didn’t regress directory cases, I can add tests for
./sub/index/to confirm we still rewrite to./sub/index/index.{ext}only when an actual directoryindex/exists. Want me to open a follow-up?
CodSpeed Performance ReportMerging #7209 will not alter performanceComparing Summary
|
There was a problem hiding this comment.
Thank you @patrickshipe . Can you please commit a changeset? https://github.com/biomejs/biome/blob/main/CONTRIBUTING.md#create-a-changeset
Done! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/petite-pans-fetch.md (1)
5-6: Enhance the changeset per repo conventions (present behaviour, example, link).Add a present-tense behaviour sentence, a small example, and a link to the PR. Optional: slightly crisper wording.
-Fixed an issue with the `useImportExtensions` rule that overcorrected explicit imports of index files. +Resolved an overcorrection in the `useImportExtensions` rule when importing explicit index files. + +Imports that explicitly reference an index file are now preserved and no longer rewritten to nested index paths. + +#### Example +```ts +// Before (incorrect rewrite) +// import "./sub/index" -> "./sub/index/index.ts" +``` +```ts +// Now +import "./sub/index"; +``` + +See PR: https://github.com/biomejs/biome/pull/7209.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.changeset/petite-pans-fetch.md(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
.changeset/*.md
📄 CodeRabbit Inference Engine (CONTRIBUTING.md)
.changeset/*.md: Create changesets withjust new-changeset; store them in.changeset/with correct frontmatter (package keys and change type).
In changeset descriptions, follow content conventions: user-facing changes only; past tense for what you did; present tense for current behavior; link issues for fixes; link rules/assists; include representative code blocks; end every sentence with a period.
When adding headers in a changeset, only use #### or ##### levels.
Files:
.changeset/petite-pans-fetch.md
🪛 LanguageTool
.changeset/petite-pans-fetch.md
[style] ~4-~4: Consider using a different verb for a more formal wording.
Context: --- "@biomejs/biome": patch --- Fixed an issue with the useImportExtensions...
(FIX_RESOLVE)
🔇 Additional comments (1)
.changeset/petite-pans-fetch.md (1)
1-3: Frontmatter looks good.Package and change type are correctly specified for a patch release.
Co-authored-by: Arend van Beelen jr. <[email protected]>
…js#7209) Co-authored-by: Arend van Beelen jr. <[email protected]>
…js#7209) Co-authored-by: Arend van Beelen jr. <[email protected]>
Summary
The
useImportExtensionsrule was incorrectly transformingimport "./sub/index"toimport "./sub/index/index.ts"- we shouldn't be adding an extraindexto the path if the import is already explicitly referencing an index file.Test Plan
Added testing for this scenario
Docs