Skip to content

add a test for #8459 #8461

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

add a test for #8459 #8461

wants to merge 1 commit into from

Conversation

lovasoa
Copy link
Contributor

@lovasoa lovasoa commented Apr 7, 2023

See #8459

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

  • 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.
  • 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

@vercel
Copy link

vercel bot commented Apr 7, 2023

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

A member of the Team first needs to authorize it.

@lovasoa
Copy link
Contributor Author

lovasoa commented Apr 7, 2023

@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:

if (is_promise(promise)) {
const current_component = get_current_component();
promise.then(value => {
set_current_component(current_component);
update(info.then, 1, info.value, value);
set_current_component(null);
}, error => {
set_current_component(current_component);
update(info.catch, 2, info.error, error);
set_current_component(null);
if (!info.hasCatch) {
throw error;
}
});
// if we previously had a then/catch block, destroy it
if (info.current !== info.pending) {
update(info.pending, 0);
return true;
}

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
Copy link
Contributor Author

lovasoa commented Apr 7, 2023

@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 :)

@dummdidumm
Copy link
Member

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 dummdidumm added this to the one day milestone Apr 11, 2023
@lovasoa
Copy link
Contributor Author

lovasoa commented Apr 11, 2023

Ok, should I close this pr, then ?

@dummdidumm
Copy link
Member

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

@Rich-Harris
Copy link
Member

Closing Svelte 4 PRs as stale — thank you (this issue was fixed in Svelte 5)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants