chore: cleanup promise handling #12284
Conversation
|
| @@ -19,7 +19,7 @@ if (DEV && BROWSER) { | |||
| let can_inspect_stack_trace = false; | |||
|
|
|||
| // detect whether async stack traces work | |||
| const check_stack_trace = async () => { | |||
| const check_stack_trace = () => { | |||
There was a problem hiding this comment.
this is there for a reason! without it, we're not actually checking anything
| const check_stack_trace = () => { | |
| const check_stack_trace = async () => { |
There was a problem hiding this comment.
perhaps we could avoid the ignore by writing it as this?
const check_stack_trace = () => {
const stack = /** @type {string} */ (new Error().stack);
can_inspect_stack_trace = stack.includes('check_stack_trace');
};
// detect whether async stack traces work
Promise.resolve(check_stack_trace());
There was a problem hiding this comment.
That completely misses the point, which is to find out whether stack traces cross async boundaries in the current browser.
There was a problem hiding this comment.
I've honestly got not idea what that means. Is this some kind of browser compatibility thing where that happens only in some browsers, but not others?
There was a problem hiding this comment.
That's the purpose of it, yes. IIRC stack traces cross async boundaries in Firefox but not Chrome, for example.
| rules: { | ||
| '@typescript-eslint/await-thenable': 'error', | ||
| // '@typescript-eslint/no-floating-promises': 'error', | ||
| // '@typescript-eslint/no-misused-promises': 'error', |
There was a problem hiding this comment.
I took a look at the errors that arise if this rule is enabled, and not a single one is valid. It's just ESLint whining pointlessly
| // '@typescript-eslint/no-misused-promises': 'error', |
There was a problem hiding this comment.
a lot more errors pop up with no-floating-promises and i haven't looked at them all yet, but of the ones i have looked at it's the same story. definitely not something we want
There was a problem hiding this comment.
I don't understand why the issues no-floating-promises is identifying shouldn't be addressed. I just pushed a commit addressing a lot of them: 743c640. We can drop it if they're really not issues. This isn't my strong point, so it's quite possible I'm missing something. I figured I'd push it though so that I can at least learn what that is as it's easier to discuss that way.
There was a problem hiding this comment.
They're really not issues, but the rule tricked you into breaking code that works perfectly well
benmccann
force-pushed
the
promise-cleanup
branch
from
bc85846
to
336b639
Compare
| @@ -341,7 +341,7 @@ async function prerender({ out, manifest_path, metadata, verbose, env }) { | |||
| if (location) { | |||
| const resolved = resolve(encoded, location); | |||
| if (is_root_relative(resolved)) { | |||
| enqueue(decoded, decode_uri(resolved), resolved); | |||
| await enqueue(decoded, decode_uri(resolved), resolved); | |||
There was a problem hiding this comment.
why are we awaiting these all? I imagine this adding up when prerendering a lot of files, hurting perf in the end, and I can't really imagine there being errors that are otherwise uncaught.
There was a problem hiding this comment.
it's such a quick action I didn't really imagine it adding up. I've updated it to enqueue them in parallel instead though to be safe
There was a problem hiding this comment.
See, stuff like this is exactly why these rules are so dangerous — I'm pretty sure this creates deadlock, and I'm pretty sure that's why the tests started failing with the commit that added this.
You have to treat ESLint like an idiot child. You can give it very simple tasks from time to time, but don't trust it with anything important.
Let's revert all this
| @@ -7,7 +7,7 @@ import { test as base, devices } from '@playwright/test'; | |||
| export const test = base.extend({ | |||
| app: async ({ page }, use) => { | |||
| // these are assumed to have been put in the global scope by the layout | |||
| use({ | |||
| await use({ | |||
There was a problem hiding this comment.
this doesn't need to be awaited. none of them do
| } | ||
| } | ||
| await Promise.all(entries_to_enqueue.map(({ path, id }) => enqueue(null, path, undefined, id))); |
There was a problem hiding this comment.
why is this better than what existed before this PR?
There was a problem hiding this comment.
it's clearer to me as it's explicit that we intend to enqueue them in parallel. it also handles rejected promise errors which would not be handled before. while it's a pretty simple method that's unlikely to fail, but everytime I come across something like that in a code review I have to review it and assess the risk that it might fail and it's easier for me to not have to do that
There was a problem hiding this comment.
Rejected promise errors are handled already. That's what the await q.done() two lines later is for. This is duplicative and less efficient, it adds nothing!
|
Opened #12297 which adds the |
imagesfrom route-level config #12280