[no-misused-promises] async react callback properties wh 8000 en void expected #4619
Comments
|
As you mentioned - this is a correct report as there is a potential bug due to the dangling promise left behind. It's hard because it's not really possible for us to detect this case of "acceptable" usage. So it's really, really hard to allowlist some subset of the cases. Maybe this new logic should be put behind an option instead to allow people to opt-out of it? Side note that you can also use this style to mark the promise as purposely dangling (which is more easily greppable, and is supported in our <div
onClick={
() => {
void doSomethingAsync();
}
}
onClick={
() => {
void (async () => {
const x = await doSomethingAsync();
doSomethingElse(X);
})();
}
}
/> |
|
Thank you for responding @bradzacher. I'm thinking about writing a utility function for this purpose: function wrapAsyncFunction<ARGS extends unknown[]>(fn: (...args: ARGS) => Promise<unknown>): (...args: ARGS) => void {
return (...args) => {
void fn(...args);
};
}So we can use it this way: <button type="button" onClick={wrapAsyncFunction(doSomethingAsync)}>
action
</button> I'm not a big fan of |
|
I'm a fan of being verbose and descriptive, so I'd do something like |
Agreed, this is a pretty common pattern for state libraries such as React. Agreed the rule is technically correct but in the spirit of not being a pain for users I'll add an option to disable this specific check. |
|
Oh, actually, a Thanks for the report @jlowcs! I'm going to go ahead and close this issue as working as intended, but if you do have a repro please do post back. |
|
@JoshuaKGoldberg indeed, |
|
Do you have an example of those? If there's a good delineation between pre-change issues and post-change, we could always take >=1 rule option. (or maybe just add rule options for everything in the change?) |
|
I'm having the same issue as OP. React code like Also, since the error text is something specific to this new kind of reported error ( |
|
@JoshuaKGoldberg https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/no-misused-promises.md#checksvoidreturn-true provides some examples that we'd still like to be able to benefit from :) |
|
Got it, that makes sense - thank you two! Marking as accepting PRs to add in an opt-out option for each of the new error locations added in https://github.com/typescript-eslint/typescript-eslint/pull/4541/files#diff-dd469c96cb4a7fd7e266d9957762126ca0e669b65eb00173edcf40d1dcdced6fR33. This seems to be a hot issue so I'll go ahead and send that PR later today. I feel responsible for this as I should have caught the significance of the breaking changes in review 🙂. |
|
Thank you @JoshuaKGoldberg, I really appreciate it :) |
|
For the record, this also breaks async Mocha tests, e.g. |
Repro
{ "rules": { "no-misused-promises": ["error"] } }or
Expected Result
No linting error.
Actual Result
A linting error is being reported:
One way to avoid this error would be to wrap the callback's content in an async IIFE:
But that's not really practical when the callback is an existing function (like the 2nd example above).
Additional Info
I guess this new behavior is kind of expected, but it's probably gonna breaks linting on a lot of React projects (including ours, which has now over 100 erros following this update). I'll welcome any suggestion as to how to address it without simply disabling the rule (and risk missing some potential bugs that were detected before).
Versions
@typescript-eslint/eslint-plugin5.13.0@typescript-eslint/parser5.13.0TypeScript4.5.5ESLint8.10.0node16.13.2The text was updated successfully, but these errors were encountered: