feat: [extensions] add error message instead of failing silently #403
Conversation
|
WalkthroughA runtime check was added to the autofix function for adding missing file extensions in import paths. The function now verifies that the computed replacement import path differs from the original before applying the fix; if unchanged, it returns null to avoid silent no-op fixes. A new test case was added to confirm this behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ESLintRule
participant Fixer
User->>ESLintRule: Triggers autofix for missing extension
ESLintRule->>ESLintRule: Compute newImportPath
ESLintRule->>ESLintRule: Compare newImportPath with original
alt newImportPath is unchanged
ESLintRule->>User: Return null (no fix applied)
else newImportPath is different
ESLintRule->>Fixer: Apply replacement with newImportPath
end
Possibly related PRs
Suggested labels
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.29.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js test/rules/extensions.spec.tsOops! Something went wrong! :( ESLint: 9.29.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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learningstest/rules/extensions.spec.ts (6)⏰ Context from checks skipped due to timeout of 90000ms (20)
🔇 Additional comments (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: |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to d587c0f in 1 minute and 13 seconds. Click for details.
- Reviewed
26lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft 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:378
- Draft comment:
Throwing an error in the fixer (when newImportPath === source.raw) can abruptly halt ESLint execution. Consider handling this case more gracefully (e.g. reporting a configuration error via context.report) instead of throwing an exception. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a tricky case. On one hand, throwing errors in fixers can be disruptive. However, this seems like a legitimate error case where the fixer cannot proceed - it's a configuration issue that needs user attention. Using context.report here wouldn't make sense because we're already inside a fix function that was called as part of handling a reported issue. The error message is also very helpful in explaining exactly what the user needs to do. I could be wrong about the impact of throwing errors in fixers - maybe it does cause more serious problems than I realize. Also, there might be a more graceful way to handle this that I'm not seeing. While throwing errors in fixers isn't ideal, in this case it's appropriate - this is a true error condition where the fixer cannot proceed, and the error message provides clear guidance on how to fix it. Using context.report wouldn't help since we're already in a fix function. The comment should be deleted. The current error-throwing approach is appropriate given the context, and the suggested alternative wouldn't work well in this situation.
Workflow ID: wflow_RJPh9kdQWje85H4s
You can customize 
|
I spent some time today working on this, but got stuck.
fix(fixer: TSESLint.RuleFixer) {
const newImportPath = replaceImportPath(
source.raw,
fixedImportPath,
)
return source.raw === newImportPath
? null // Surface that something has gone wrong with extension detection.
: fixer.replaceText(source, newImportPath)
},But now I am confused, because returning
tInvalid({
name: 'extensions should not autofix when the fix is identical',
code: 'import foo from "./unresolved"',
options: ['always', { fix: true }],
errors: [
{
messageId: 'missing',
data: { extension: 'js', importPath: './unresolved' },
},
],
output: null,
}),But this doesn't work, I'm not really sure why. (By "not work", I mean that it passes by default, but when I remove my new code, it still passes.) I'm not really sure how to test for a |
|
So my guess could be wrong, I'll take deeper look ASAP. |
Related to #402 but does not close it.
Important
In
extensions.ts,fix()now throws an error with a message ifreplaceImportPath()fails to change the import path, aiding debugging.extensions.ts,fix()infixOrSuggestnow throws an error ifreplaceImportPath()does not change the import path.This description was created by
for d587c0f. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Summary by CodeRabbit