chore: validate promise handling #11842
Conversation
|
|
if we wanted to add a way to detect unhandled promise rejections, would it have to be in form of a hook? forced console.error are things people don't tend to like and then have to bring in bundlers that filter them. |
|
While it can't bring down the server, it would make it pretty hard to deal if there was ever an error and it wasn't reported in any way. I ignored instances of failing that check for now. Is it worth filing an issue for the hook that @dominikg mentioned? |
benmccann
force-pushed
the
floating-promises
branch
from
16aaa2a to
73eff42
Compare
benmccann
changed the title
benmccann
force-pushed
the
floating-promises
branch
from
d758361 to
10ac6aa
Compare
…returning promises
benmccann
force-pushed
the
floating-promises
branch
from
10ac6aa to
0611bef
Compare
| @@ -20,7 +20,7 @@ export function slice_source(code_slice, offset, { file_basename, filename, get_ | |||
| * @param {(...match: any[]) => Promise<MappedCode>} get_replacement | |||
| * @param {string} source | |||
| */ | |||
| function calculate_replacements(re, get_replacement, source) { | |||
| async function calculate_replacements(re, get_replacement, source) { | |||
There was a problem hiding this comment.
I don't understand. What is this for?
There was a problem hiding this comment.
Because it returns a promise
There was a problem hiding this comment.
(to be clear: I understand that eslint fails because of @typescript-eslint/promise-function-async, but that rule seems particularly pointless even by the standards of the genre)
There was a problem hiding this comment.
(in fact i'm pretty sure that making a function async unnecessarily creates runtime overhead. i just have no idea why we would do this to ourselves)
There was a problem hiding this comment.
All of the ones that would still return a promise but don't have an await in them, probably, yeah.
There was a problem hiding this comment.
Yes, absolutely. But to be honest I question the value of any of it. We need less naggy ESLint bullshit, not more. The fact that you have to add this...
// eslint-disable-next-line @typescript-eslint/no-floating-promises...to tell the plugin that Promise.resolve() — of all things! — doesn't need a .catch handler tells you all you need to know about how much value it adds.
There was a problem hiding this comment.
I still don't understand the benefit of leaving it off
As mentioned, it creates runtime overhead. AFAIK this will result in four promises being created rather than two:
async function get(thing) {
return fetch(`${base}/${thing}`).then(async (r) => r.json());
}There was a problem hiding this comment.
...to tell the plugin that Promise.resolve() — of all things! — doesn't need a .catch handler tells you all you need to know about how much value it adds.
I believe that the issue isn't Promise.resolve(), but that an exception could be thrown in .then() and then it would be unhandled
There was a problem hiding this comment.
Either way it's an annoying rule that we really don't need. I got rid of the ones that don't provide any value (or provide negative value, as in the case of promise-function-async)
|
|
|
This is the sample from no-misused-promises with TypeScript by itself — it seems pretty redundant? 
I can concede that |
I don't know whether
console.erroris the best way to deal with these or not. Please feel free to fix this up if you have better ideashttps://typescript-eslint.io/rules/no-floating-promises/
I wanted to turn these rules on because they catch tons of errors in SvelteKit. Turning them on here as well will let us add them to our common config. I fixed several dozen errors in SvelteKit with several dozen left to go in sveltejs/kit#12284