svelte icon indicating copy to clipboard operation
svelte copied to clipboard

add a test for #8459

Open lovasoa opened this issue 2 years ago • 6 comments

See https://github.com/sveltejs/svelte/issues/8459

Before submitting the PR, please make sure you do the following

  • [x] It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • [ ] Prefix your PR title with feat:, fix:, chore:, or docs:.
  • [ ] This message body should clearly illustrate what problems it solves.
  • [x] Ideally, include a test that fails without this PR but passes with it.

Tests

  • [ ] Run the tests with npm test and lint the project with npm run lint

lovasoa avatar Apr 07 '23 09:04 lovasoa

@lovasoa is attempting to deploy a commit to the Svelte Team on Vercel.

A member of the Team first needs to authorize it.

vercel[bot] avatar Apr 07 '23 09:04 vercel[bot]

@benmccann : I see you re-triggered the tests, but they are supposed to fail, in the current state of this PR. It just adds a test to the test suite that illustrates the bug reported in #8459 .

I am willing to help with solving the bug, but I would need some pointers. Looking at the code, I guess the issue is here:

https://github.com/sveltejs/svelte/blob/d42ca041dd7817ae772a9da2d0aea56f557088d1/src/runtime/internal/await_block.ts#L82-L101

I can see that that the block always immediately changes to a pending state when the function runs, even if the given promise is already resolved.

But I do not grasp what exactly is the role of each field in PromiseInfo and what invariants about it handle_promise is supposed to hold.

On a broader level, I am not sure I understand why a special AwaitBlockWrapper is needed. Couldn't the compiler just compile the {#await} syntax down to three if blocks and two variables storing the data and the error in the promise ?

lovasoa avatar Apr 07 '23 18:04 lovasoa

@Rich-Harris @tanhauhau , I see you are the latest contributors to this part of the code, I would love if you could give me some pointers too :)

lovasoa avatar Apr 07 '23 18:04 lovasoa

The underlying issue is that promises that are resolved already are still treated as "need to render the await first and then resolve once the result is there". We want to look into changing this, but that won't happen before Svelte 5.

dummdidumm avatar Apr 11 '23 11:04 dummdidumm

Ok, should I close this pr, then ?

lovasoa avatar Apr 11 '23 12:04 lovasoa

I added the "one day" label instead of closing so it has visibility, no further action needed from your side 👍

dummdidumm avatar Apr 11 '23 12:04 dummdidumm