feat: correct preserve-caught-error edge cases
#20109
Conversation
✅ Deploy Preview for docs-eslint ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
lib/rules/preserve-caught-error.js
Outdated
| // Multiple cause properties found, no reliable suggestion can be made | ||
|
|
||
| context.report({ | ||
| messageId: "incorrectCause", |
There was a problem hiding this comment.
I'm not sure if there should be a different error message when cause is defined multiple times.
There was a problem hiding this comment.
Yeah, the The symptom error is being thrown with an incorrect `cause`. message would be confusing in the following case because the error is actually thrown with the correct cause:
/* eslint preserve-caught-error: "error" */
try {
trySomething();
} catch (error) {
throw new Error("Something's broken", {
cause: bar,
cause: error,
});
}We could either:
- Use a different message saying that
causeis defined multiple times, though it would overlap with theno-dupe-keysrule. - Not report any errors, because this is more a case for the
no-dupe-keysrule. - Check the last
causeproperty definition only because that's the one that will be set on the object.
I think I'd vote for 2.
There was a problem hiding this comment.
Probably a good idea not to report an error when the last cause has the expected value. It seems that the current message should be clear enough in other cases.
There was a problem hiding this comment.
If we know the last cause has the expected value, then I would expect that we'd go off of that one. Whether a project has no-dupe-keys enabled doesn't impact whether the issues caught by this rule are impactful. I vote for 3. +1 that the current message should be clear enough in other cases.
There was a problem hiding this comment.
Thanks for the suggestions! I've updated the rule to check the last cause definition and only provide suggestions when a single definition is present. If we decide to ignore cases that overlap with no-dupe-keys altogether, we'll have to make an exception for getter and setter pairs, which aren't reported, and account for other edge cases of that rule, so I thought that checking the last definition would keep the logic simple.
preserve-caught-error edge casespreserve-caught-error edge cases
| { | ||
| code: `try {} catch (error) { | ||
| throw new Error("Something failed", { cause: error, cause: anotherError }); | ||
| }`, | ||
| errors: [{ messageId: "incorrectCause" }], | ||
| }, |
There was a problem hiding this comment.
This was valid before this change, so I've changed the tag to feat because this change can produce more lint errors.
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
Tell us about your environment (
npx eslint --env-info):Node version: v22.19.0
npm version: v10.9.3
Local ESLint version: v9.35.0 (Currently used)
Global ESLint version: v9.35.0
Operating System: darwin 24.6.0
What parser are you using (place an "X" next to just one item)?
[x]
Default (Espree)[ ]
@typescript-eslint/parser[ ]
@babel/eslint-parser[ ]
vue-eslint-parser[ ]
@angular-eslint/template-parser[ ]
OtherPlease show your full configuration:
inline config only
What did you do? Please include the actual source code causing the issue.
Playground link
Playground link
What did you expect to happen?
In the first example with the getter, the suggestion should produce valid code but it doesn't.
In the second example, the rule should report an error because the cause value will be
bar.What actually happened? Please include the actual, raw output from ESLint.
In the first example, the suggested fix is producing invalid JavaScript.
In the second example, no error is being reported.
What changes did you make? (Give an overview)
This pull request ensures that if there are multiple
causedefinitions, only the last one is checked and no suggestions are provided. The suggestion for getters and setters has been fixed.Is there anything you'd like reviewers to focus on?