-
-
Notifications
You must be signed in to change notification settings - Fork 53
fix: do not consider resolved path in extensions rule #374
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: do not consider resolved path in extensions rule #374
Conversation
WalkthroughThe change reverses the priority for extracting file extensions from import paths. It now first attempts to get the extension from the original import path string, and only if that fails, it falls back to using the resolved import path. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/rules/extensions.tsOops! Something went wrong! :( ESLint: 9.28.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 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 (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
commit: |
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.
Important
Looks good to me! 👍
Reviewed everything up to 5a078d5 in 49 seconds. Click for details.
- Reviewed
17
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/rules/extensions.ts:326
- Draft comment:
Using the raw importPath to extract the extension now aligns with the intended behavior described in the PR. Verify that this change correctly handles edge cases (e.g. TS files resolved from .js imports) and that tests are updated accordingly. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_5f04JNX6JDw7WJ5U
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Tests are failing, or maybe you can raise a new issue or a PR with failing test case first. |
|
@@ -328,9 +328,9 @@ export default createRule<Options, MessageId>({ | |||
|
|||
const resolvedPath = resolve(importPath, context) | |||
|
|||
// get extension from resolved path, if possible. | |||
// for unresolved, use source value. | |||
const extension = path.extname(resolvedPath || importPath).slice(1) |
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.
This change is incorrect for resolving .xyz
but ./abc.js
or ./abc
is imported.
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/rules/extensions.ts
(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on windows-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on ubuntu-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on ubuntu-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on ubuntu-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on macos-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on ubuntu-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on ubuntu-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on ubuntu-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on macos-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on ubuntu-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on macos-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on ubuntu-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on macos-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on macos-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on macos-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on ubuntu-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on macos-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on macos-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on macos-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on macos-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on macos-latest
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Check: publish
src/rules/extensions.ts
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Actions: CI
src/rules/extensions.ts
[error] 333-333: TypeScript error TS2345: Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Actions: Publish Any Commit
src/rules/extensions.ts
[error] 333-333: TypeScript error TS2345: Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🔇 Additional comments (1)
src/rules/extensions.ts (1)
331-333
:❓ Verification inconclusive
Verify the impact on existing functionality.
The logic change from prioritizing resolved path extension to import path extension aligns with the TypeScript migration use case. However, the failing tests mentioned in the PR comments suggest this change may have broader implications.
Please run the following script to understand which tests are failing and their expected behavior:
🏁 Script executed:
#!/bin/bash # Check test files related to extensions rule echo "=== Finding extension-related test files ===" fd -e js -e ts test | grep -i extension echo -e "\n=== Searching for test cases that might be affected ===" rg -A 10 -B 5 "resolvedPath|resolved.*path" --type ts --type js echo -e "\n=== Looking for existing test cases with import/resolved path discrepancies ===" rg -A 15 -B 5 "\.js.*\.ts|\.ts.*\.js" --type ts --type jsLength of output: 31446
Verify extension resolution logic and add coverage tests
I didn’t find any existing specs exercising the new prioritization of
importPath
’s extension overresolvedPath
(noextensions
-named tests were detected). Please confirm that this change won’t regress existing behavior and consider adding explicit test cases for:
- src/rules/extensions.ts (lines 331–333)
const extension = path.extname(importPath)?.slice?.(1) || path.extname(resolvedPath).slice(1)- Scenarios where
importPath
andresolvedPath
have different extensions (e.g..ts
vs..js
), and whenimportPath
lacks an extension.- Fallback behavior when
importPath
has no sliceable extension.Add or update tests under
test/rules/extensions.spec.ts
(or similarly named) to cover these cases.🧰 Tools
🪛 GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on windows-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on windows-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on windows-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on windows-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on windows-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on windows-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on ubuntu-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on ubuntu-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on windows-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on windows-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on ubuntu-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on macos-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on windows-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on ubuntu-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on ubuntu-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on ubuntu-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on macos-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on ubuntu-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on macos-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on ubuntu-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on macos-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on macos-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 24 and ESLint 8.56 on macos-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on ubuntu-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on macos-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on macos-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on macos-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on macos-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on macos-latest
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Check: publish
[failure] 333-333:
Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.🪛 GitHub Actions: CI
[error] 333-333: TypeScript error TS2345: Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
🪛 GitHub Actions: Publish Any Commit
[error] 333-333: TypeScript error TS2345: Argument of type 'string | null | undefined' is not assignable to parameter of type 'string'.
…ath doesn't have an extension
I just ran into the following issue:
I'm migrating to TypeScript and I have
import foo from './foo.js';
in a file and there isfoo.ts
. I also have'import-x/extensions': ['error', 'ignorePackages', { js: 'never', ts: 'never' }]
configured so that the dev omits.js
and then the resolver will correctly resolvefoo.ts
. But this is not the case because eslint-plugin-import-x will only suggest to omit the extension if the extension of the import statement is equal to the resolved path.My suggestion is to not
||
the paths but the extensions. So, if the import path has an extension, take this, otherwise take the one from the resolved path.Yeah, I think the solution isn't ideal yet, there are failing tests. Maybe someone can help with it.
EDIT: Closed in favor of
extensionAlias
option from eslint-import-resolver-typescript which prevents the case where import path and resolved path are not aligned.Important
Change extension determination in
extensions.ts
to useimportPath
directly, affecting ESLint extension rules in TypeScript projects.extensions.ts
, change extension determination to useimportPath
directly instead ofresolvedPath
.createRule
inextensions.ts
to usepath.extname(importPath).slice(1)
for extension extraction.resolvedPath
variable inextensions.ts
.This description was created by
for 5a078d5. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit