Skip to content

await blocks unnecessarily unmount and remount their children when the promise changes #8459

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
lovasoa opened this issue Apr 7, 2023 · 7 comments · Fixed by #11989
Closed
Assignees
Milestone

Comments

@lovasoa
Copy link
Contributor

lovasoa commented Apr 7, 2023

Describe the bug

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.

Reproduction

https://svelte.dev/repl/b70b678cc4304ccdac16419e6a979168?version=3.58.0

<script>
  	import ShowValue from "./ShowValue.svelte"
	
	const promise_a = Promise.resolve('a')
	const promise_b = Promise.resolve('b')

	let current_promise = promise_a;
</script>

{#await current_promise then value}
	<ShowValue {value} />
{/await}

<button on:click={()=>{current_promise = promise_a}}>Show Promise A</button>
<button on:click={()=>{current_promise = promise_b}}>Show Promise B</button>

Here, I wouldn't expect ShowValue to be unmounted and remounted when the buttons are clicked.
In more complex cases, when ShowValue is expensive to mount (when it has to do many DOM operations, for instance), this becomes a performance problem.

Logs

No response

System Info

Tested in svelte 3.58.0

Severity

annoyance

lovasoa added a commit to lovasoa/svelte that referenced this issue Apr 7, 2023
@lovasoa lovasoa mentioned this issue Apr 7, 2023
5 tasks
@lovasoa
Copy link
Contributor Author

lovasoa commented Apr 7, 2023

The bug only appears with the then and error blocks, but not in the pending block. Replacing a pending promise with another pending promise does not unmount and remount the pending block.

@lovasoa
Copy link
Contributor Author

lovasoa commented Apr 7, 2023

I wrote a small component to use instead of the native {#await} that works around this problem

<script>
	const PENDING = {};
	let data = PENDING;
	let error = PENDING;
	
	export let promise;
	
	async function onChange() {
		[data, error] = await Promise.race([promise, PENDING]).then(
			d => [d, PENDING],
			e => [PENDING, e],
		);
		if (data === PENDING && error === PENDING) promise.then(onChange, onChange)
	}

	$: onChange(promise)
</script>

{#if data !== PENDING}
	<slot {data} />
{:else if error !== PENDING}
	<slot name="error" {error} />
{:else}
	<slot name="pending"/>
{/if}

It can be used like so

<Await {promise} let:data>
	Resolved with {data}
	<div slot="pending">Pending...</div>
	<div slot="error" let:error>Error: {error}</div>
</Await>

@Rich-Harris
Copy link
Member

The current behaviour isn't buggy per se, but it probably would be better to delay the update by a single microtask when changing from one promise to another, in case the latter resolves in that window. Adding the 5.0 milestone so we can review (it is technically a breaking change so would need to happen in a major)

@Rich-Harris Rich-Harris added this to the 5.0 milestone Apr 1, 2024
@trueadm trueadm self-assigned this Jun 4, 2024
@trueadm
Copy link
Contributor

trueadm commented Jun 10, 2024

After looking into this, I'm not sure delaying by a microtask gives us anything. This is a bit like a key block in that the promise is different, so we unmount and remount. I think the existing behaviour actually makes sense here.

@lovasoa
Copy link
Contributor Author

lovasoa commented Jun 10, 2024

Maybe it's just me (and the 5 people that upvoted this issue), but I found the current behavior impractical. The way it manifests in applications is most often with intempestive loading spinners and unneeded backend requests being made. Fixing this would, I think, just make applications snappier in most cases, and I don't see any obvious downside to it.

@trueadm
Copy link
Contributor

trueadm commented Jun 10, 2024

Let me take another look at what we can do then.

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 a pull request may close this issue.

4 participants