linter(import/extensions): improve functionality#15009
linter(import/extensions): improve functionality#15009wagenet wants to merge 2 commits intooxc-project:mainfrom
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
CodSpeed Performance ReportMerging #15009 will not alter performanceComparing Summary
Footnotes
|
There was a problem hiding this comment.
Pull Request Overview
This PR improves the import/extensions rule to better align with ESLint's behavior while accounting for oxc's lack of file resolution capabilities. The changes introduce support for wildcard patterns, path group overrides, and more accurate package detection heuristics.
Key Changes:
- Added support for wildcard
*pattern andpathGroupOverridesconfiguration options - Improved package detection logic to distinguish between scoped packages, path aliases, and relative imports
- Enhanced extension checking logic with better handling of modern module extensions (.mts, .cts, etc.)
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| crates/oxc_linter/src/rules/import/extensions.rs | Core implementation changes including new data structures, improved package detection, wildcard pattern support, and path group override functionality |
| crates/oxc_linter/src/snapshots/import_extensions.snap | Updated test snapshots reflecting the improved rule behavior and new test cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| Self { | ||
| ignore_packages: true, | ||
| ignore_packages: false, // ESLint default is false |
There was a problem hiding this comment.
The comment states ESLint's default is false, but this changes the previous default from true to false. This is a breaking change that could affect existing users. Consider documenting this breaking change or maintaining backward compatibility.
| ignore_packages: false, // ESLint default is false | |
| ignore_packages: true, // Previous default was true; ESLint default is false |
There was a problem hiding this comment.
Fair point, but my take is that the previous behavior was incorrect and therefore a bug.
| if ignore_packages && !should_enforce { | ||
| return; | ||
| } | ||
| // Even without ignore_packages, we can't reliably extract extensions from package names |
There was a problem hiding this comment.
This logic has two consecutive early returns that always execute. Lines 409-410 will always return regardless of the condition on line 406, making lines 406-408 unreachable in practice. Simplify to a single unconditional return since package root names without subpaths are always skipped.
| if ignore_packages && !should_enforce { | |
| return; | |
| } | |
| // Even without ignore_packages, we can't reliably extract extensions from package names | |
| // Always skip: can't reliably extract extensions from package names |
| if matches!( | ||
| key.as_str(), | ||
| "ignorePackages" | "checkTypeImports" | "pattern" | "pathGroupOverrides" | ||
| ) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
The parser skips the "pattern" key in configuration parsing, but tests at lines 780, 788, 987, and 1000 use "pattern" with nested extension configurations (e.g., { "pattern": { "*": "always", "js": "never" } }). This causes those test configurations to be silently ignored.
Impact: Tests will pass with incorrect behavior. The pattern key functionality is not implemented but tests assume it works.
Fix: Either remove "pattern" from the skip list and implement proper parsing, or update the tests to not use nested "pattern" keys:
// Option 1: Remove "pattern" from skip list and handle it
if matches!(
key.as_str(),
"ignorePackages" | "checkTypeImports" | "pathGroupOverrides"
) {
continue;
}
// Then add pattern handling before extension parsing
if key == "pattern" {
if let Some(pattern_obj) = val.as_object() {
for (ext_key, ext_val) in pattern_obj {
if let Some(config_str) = ext_val.as_str() {
extensions.insert(ext_key.clone(), FileExtensionConfig::from(config_str));
}
}
}
continue;
}| if matches!( | |
| key.as_str(), | |
| "ignorePackages" | "checkTypeImports" | "pattern" | "pathGroupOverrides" | |
| ) { | |
| continue; | |
| } | |
| if matches!( | |
| key.as_str(), | |
| "ignorePackages" | "checkTypeImports" | "pathGroupOverrides" | |
| ) { | |
| continue; | |
| } | |
| if key == "pattern" { | |
| if let Some(pattern_obj) = val.as_object() { | |
| for (ext_key, ext_val) in pattern_obj { | |
| if let Some(config_str) = ext_val.as_str() { | |
| extensions.insert(ext_key.clone(), FileExtensionConfig::from(config_str)); | |
| } | |
| } | |
| } | |
| continue; | |
| } |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
This doesn't seem to be strictly necessary.
Since we depend on the pathGroupOverrides config item (for e.g. ~icons/*), we need to wait until oxc-project/oxc#15009 is released
Since we depend on the pathGroupOverrides config item (for e.g. ~icons/*), we need to wait until oxc-project/oxc#15009 is released
155adc7 to
bd3768b
Compare
|
See also #14602, I'm not sure which is the best to go with as the solution here but I definitely think we need to prioritize fixing this rule. |
|
Also this issue #12220, which I think this PR would solve? |
|
#14602 has been merged, so this PR is probably not necessary anymore. If there are further fixes that are desired, please feel free to open a new PR :) |
This PR brings things more in line with the ESLint plugin's behavior here. However, we can't have full parity since the ESLint plugin relies upon file resolution. Where it did rely on this we try to err towards being permissive to avoid false positives.
This was primarily written with AI, though the code seems reasonable as far as I can tell.