Skip to content

fix: wait a microtask for await blocks to reduce UI churn #11989

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

Merged
merged 22 commits into from
Jun 20, 2024

Conversation

trueadm
Copy link
Contributor

@trueadm trueadm commented Jun 10, 2024

The PR changes await blocks, to avoid showing/hiding the pending/then state of an each block if the promise resolves by the next microtask. Fixes #8459.

Copy link

changeset-bot bot commented Jun 10, 2024

🦋 Changeset detected

Latest commit: 99dcc68

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
svelte Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@dummdidumm
Copy link
Member

Are you sure this works? I still get the pending block logged in this example

@trueadm
Copy link
Contributor Author

trueadm commented Jun 11, 2024

Are you sure this works? I still get the pending block logged in this example

This PR aims to reduce the amount of churn on pending blocks. The then block is an entirely different beast that will involve majorly refactoring how the then block works to use signals. Right now it works a bit like the key block.

@dummdidumm
Copy link
Member

dummdidumm commented Jun 11, 2024

The referenced issue states:

When the promise of an {#await ... } block changes from a resolved Promise to another resolved Promise, svelte unnecessarily unmounts and then remounts all the contents of the "then" fragment of the await block.

... which is what is still happening here. Switching from one resolved promise to another is the main thing that people complain about.

@trueadm
Copy link
Contributor Author

trueadm commented Jun 11, 2024

The referenced issue states:

When the promise of an {#await ... } block changes from a resolved Promise to another resolved Promise, svelte unnecessarily unmounts and then remounts all the contents of the "then" fragment of the await block.

... which is what is still happening here. Switching from one resolved promise to another is the main thing that people complain about.

I've updated the PR to reflect that this isn't solving that issue anymore.

@dummdidumm
Copy link
Member

But what is this PR really changing then? The pending block is still rendered on promise change, just the effect isn't fired because it's cleaned up before it can run.

@trueadm
Copy link
Contributor Author

trueadm commented Jun 11, 2024

@dummdidumm The pending block isn't rendered on promise change if it only takes a microtask (except initial mount). There was a bug in your case, but I just fixed that.

@dummdidumm
Copy link
Member

With your change the referenced issue is fixed, and my playground works as expected. I assume this was a "order in the task queue" issue and adding the .then before queuing the microtask fixed it?

@Rich-Harris
Copy link
Member

Why render the pending block on mount, if the promise is already resolved? Opened #11995 as a proof of concept of not doing that

@trueadm trueadm marked this pull request as draft June 11, 2024 15:37
@trueadm
Copy link
Contributor Author

trueadm commented Jun 11, 2024

Converted this to a draft. I think I can get then blocks to also not remount too.

Update: ready now!

@Rich-Harris
Copy link
Member

The pending block is destroyed and recreated if you go from one unresolved promise to another. Working on it locally

@Rich-Harris Rich-Harris merged commit 6a3e293 into main Jun 20, 2024
9 checks passed
@Rich-Harris Rich-Harris deleted the better-await-pending branch June 20, 2024 19:26
This was referenced Jun 20, 2024
FoHoOV pushed a commit to FoHoOV/svelte that referenced this pull request Jun 27, 2024
…1989)

* fix: wait a microtask for await blocks to reduce UI churn

* fix: wait a microtask for await blocks to reduce UI churn

* fix: wait a microtask for await blocks to reduce UI churn

* fix bug

* Make then blocks reactive

* add test

* update test

* update test

* Update packages/svelte/src/internal/client/dom/blocks/await.js

Co-authored-by: Simon H <[email protected]>

* Add support for catch block

* slightly more specific naming

* if we use the reserved $$ prefix we dont need to mess around with scope.generate

* omit args for then/catch if unnecessary

* neaten up some old code

* shrink code

* simplify test

* add failing test

* preserve pending blocks

* update test

* fix comment typo

* tidy up

---------

Co-authored-by: Simon H <[email protected]>
Co-authored-by: Rich Harris <[email protected]>
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.

await blocks unnecessarily unmount and remount their children when the promise changes
4 participants