extensions: treat relative package roots as packages for ignorePackages#3261
extensions: treat relative package roots as packages for ignorePackages#3261morgan-coded wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates the extensions rule to avoid false positives when ignorePackages is enabled and a relative import resolves to a directory that is itself a package root (has its own package.json), aligning behavior with issue #2844.
Changes:
- Added logic to detect relative specifiers that resolve to a package root directory and treat them as packages.
- Updated rule tests to mark these relative package-root imports as valid under
ignorePackages. - Documented the fix in the changelog and added the issue reference link.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/src/rules/extensions.js | Moves previously-failing relative package-root cases into valid for ignorePackages. |
| src/rules/extensions.js | Adds isRelativeToPackage() and integrates it into the “is this a package?” decision. |
| CHANGELOG.md | Notes the fix and adds a link to issue #2844. |
Comments suppressed due to low confidence (1)
src/rules/extensions.js:1
- Operator precedence here makes
isScoped(importPath)andisRelativeToPackage(importPath, context)apply even whenignorePackagesis false. That causes scoped imports / relative package-root imports to be treated as packages unconditionally, which can incorrectly skip extension enforcement. Wrap the OR-conditions inside theignorePackages && (...)group so the package-ignoring behavior only activates whenignorePackagesis enabled.
import path from 'path';
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function isRelativeToPackage(importPath, context) { | ||
| if (!(/^[.]{1,2}([\\/]|$)/).test(importPath)) { | ||
| return false; | ||
| } | ||
| const physicalFilename = getPhysicalFilename(context); | ||
| if (!physicalFilename || physicalFilename === '<text>') { | ||
| return false; | ||
| } | ||
| const targetPath = path.resolve(path.dirname(physicalFilename), importPath); | ||
| try { | ||
| return getFilePackagePath(targetPath) === targetPath; | ||
| } catch (e) { | ||
| return false; | ||
| } | ||
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3261 +/- ##
==========================================
- Coverage 95.82% 95.74% -0.09%
==========================================
Files 83 83
Lines 3741 3759 +18
Branches 1355 1359 +4
==========================================
+ Hits 3585 3599 +14
- Misses 156 160 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ljharb
left a comment
ljharb
left a comment
There was a problem hiding this comment.
I'm a bit skeptical of non-breaking-change PRs that change existing tests. can you try to minimize the diff on existing tests?
|
That makes sense. I reworked the tests so the existing TODO cases are just moved verbatim from invalid to valid, with the extra fallback coverage added separately. Focused suite, full tests-only, lint, diff-check, and hygiene are all clean. |
morgan-coded
force-pushed
the
fix/2844-extensions-relative-package-ignore
branch
from
080a364 to
e9a634b
Compare
Problem I saw:
import/extensionscould still report.or..as missing an extension even whenignorePackageswas enabled.Those relative imports can point at a directory that is itself a package root, but the rule only gave package treatment to external or scoped imports. Thanks @benasher44 for filing the follow-up from #2778.
What I changed:
I added a narrow package-root check for relative
.and..imports.When one of those specifiers resolves to a package root, it now follows the same
ignorePackagesbehavior as package imports. I kept the scope toignorePackagesand did not broaden the ambiguousalwaysplus relative-import case.How I checked it:
I moved the existing TODO cases into valid first and got the expected red run: 69 passing, 2 failing.
After the fix, the
extensionsrule suite passed with 71 passing and 0 failing. The full repo suite also passed with 3029 passing, 0 failing, and 1 pending. Lint passed on the changed files, and pre-push hygiene is clean.Closes #2844