Skip to content
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

reactive variable using $state breaks when updated inside #await #13817

Closed
tommasoiovane opened this issue Oct 23, 2024 · 20 comments · Fixed by #13877 or #13819
Closed

reactive variable using $state breaks when updated inside #await #13817

tommasoiovane opened this issue Oct 23, 2024 · 20 comments · Fixed by #13877 or #13819

Comments

@tommasoiovane
Copy link

Describe the bug

I'm trying to migrate a svelte 4 app to version 5 but it seems that updating a variable marked with $state inside an async function called by {#await fn()} breaks the component with no errors in console.

Reproduction

I created a simple playground as you can see there's no output but if you remove $state('world') and replace it by 'world' using the "old way" it works
image

Logs

No response

System Info

System:
    OS: Linux 6.11 Debian GNU/Linux 11 (bullseye) 11 (bullseye)
    CPU: (14) x64 Intel(R) Core(TM) Ultra 7 165U
    Memory: 16.04 GB / 30.05 GB
    Container: Yes
    Shell: 5.1.4 - /bin/bash
  Binaries:
    Node: 20.18.0 - /usr/local/bin/node
    Yarn: 1.22.22 - /usr/local/bin/yarn
    npm: 10.8.2 - /usr/local/bin/npm
    pnpm: 9.12.2 - /usr/local/share/npm-global/bin/pnpm
  npmPackages:
    svelte: ^5.0.5 => 5.0.5

Severity

blocking an upgrade

@paoloricciuti
Copy link
Member

This is working as intended...you should update state from the template. Thanks for reporting tho.

@brunnerh
Copy link
Member

It breaks silently, I would hardly call that "working as intended".

@tommasoiovane
Copy link
Author

tommasoiovane commented Oct 23, 2024

This is working as intended...you should update state from the template. Thanks for reporting tho.

What is the recommended way to update a state after calling an api or any async function then?
Because, to me, this looks less practical then version 4

@tommasoiovane
Copy link
Author

also the docs https://svelte.dev/docs/svelte/$state never mentions any of this, the migration guide explicitly says that you can migrate from let to $state with no other changes: https://svelte.dev/docs/svelte/v5-migration-guide#Reactivity-syntax-changes

@paoloricciuti
Copy link
Member

It breaks silently, I would hardly call that "working as intended".

It should throw an error...if it doesn't then we should investigate that

@brunnerh
Copy link
Member

brunnerh commented Oct 23, 2024

Also would not necessarily treat the #await expression the same as any other {interpolation}.
I think it's fairly common for existing code using #await to update state/local flags, partially because #await is rather trash otherwise (awaited data would only be available in a restricted scope within the template, not in <script> code).

@paoloricciuti
Copy link
Member

Also would not necessarily treat the #await expression the same as any other {interpolation}. I think it's fairly common for existing code using #await to update state/local flags, partially because #await is rather trash otherwise (awaited data would only be available in a restricted scope within the template, not in <script> code).

Mmm @trueadm wdyt about this? Await should be less prone to create infinite loops and I think if you need the scope to be larger you can always move stuff in an effect but it could also make sense to allow this pattern.

@trueadm
Copy link
Contributor

trueadm commented Oct 23, 2024

I think the general error here is that there is an error but it's being swallowed because there's no catch block. Maybe if the error comes from Svelte and there is no catch block, then we console.error the message in DEV?

As to altering the behaviour to allow mutations – that just won't work. The expression of the catch block is a tracked context – like a derived. It's tracked so that it can read things and react to them changing. It also doesn't make a very good function to make async – and maybe we should document this more, as as soon as you await inside it, things aren't going to be tracked either.

However, because the function is async, you can use that to avoid the mutation error too, because mutations outside of the reactive context are permitted, and obviously doing await means you come out of that context. So this works:

https://svelte.dev/playground/a82cf40707aa4b0e9525fb7257a154b1?version=5.0.5

If we were to allow mutations in this logic expression, but not others, it would add to the confusion. So I'm not sure of there being any simple path solution here.

@MathiasWP
Copy link
Contributor

This is working as intended...you should update state from the template. Thanks for reporting tho.

What does update state from the template mean?

@brunnerh
Copy link
Member

brunnerh commented Oct 23, 2024

Something like this:

<script>
  let value = $state(0);
</script>

{value = 1}

The template (HTML part of the component) should only read state, not change it (unless we are talking about things like event callbacks).

@trueadm
Copy link
Contributor

trueadm commented Oct 23, 2024

I put up a PR to at least console.error the error if there's no catch block in DEV. That should help surface the problem as to why nothing is rendered.

@MathiasWP
Copy link
Contributor

Something like this:

<script>
  let value = $state(0);
</script>

{value = 1}

The template (HTML part of the component) should only read state, not change it (unless we are talking about things like event callbacks).

Then why did @paoloricciuti say "you should update state from the template"? 🤔

@paoloricciuti
Copy link
Member

Oh that's a typo sorry, you shouldn't

@tommasoiovane
Copy link
Author

If we were to allow mutations in this logic expression, but not others, it would add to the confusion. So I'm not sure of there being any simple path solution here.

I agree with the concept, but I believe the await block serves a very different purpose compared to other blocks like if or each. The await block looks specifically designed to handle asynchronous functions, and it makes sense that this function should be able to modify variables within the component.

Anyway thanks your explanation was very clear

@gyzerok
Copy link

gyzerok commented Oct 24, 2024

We are struggling with the same issue in our project and I'd like to advocate to permit mutation in templates if inside untrack block similar to how it is permitted in $derived and $effect.

In our project we are using a sort of a read-through cache which returns value if it's already cached or fires a request and returns some "loading" indicator if not.

In the following code (REPL) things would work if we swap untrack for tick. This code roughly represents our logic. In our case though promise will make an actual API request.

<script lang="ts">
	import { untrack, tick } from 'svelte'
	import { SvelteMap } from 'svelte/reactivity'

	class Cache {
		private cache: SvelteMap<number, string | Promise<string>>;

		constructor() {
			this.cache = new SvelteMap()
		}

		getAsync = (id: number): string | Promise<string> => {
			const model = this.cache.get(id)

			if (!model) {
				const promise = new Promise(resolve =>
					setTimeout(() => {
						this.cache.set(id, id.toString())
					}, 1000)
			  ).then(() => this.cache.get(id));

				untrack(() => {
					this.cache.set(id, promise)
				})
				// tick().then(() => {
				// 	this.cache.set(id, promise)
				// })
				
				return promise;
			}

			return model;
		}
	}

	const cache = new Cache()
</script>

<div>
	{cache.getAsync(1)}
</div>

@trueadm
Copy link
Contributor

trueadm commented Oct 24, 2024

@gyzerok That example seems to work fine if you put it in runes mode. You just need to ensure you're in runes mode – which that example is not.

@gyzerok
Copy link

gyzerok commented Oct 24, 2024

@trueadm you are right, sorry for that. Can you please check this modified REPL where I end up in the inifinite loading state when using untrack?

@trueadm
Copy link
Contributor

trueadm commented Oct 24, 2024

This is an odd one, but the reason for it is because SvelteMap is lazy – so it only creates signals for properties when they exist. Because it doesn't exist when you read it doing this const model = this.cache.get(id), it tries to read it when you do this.cache.set(id, promise). However, you've put it in an untrack so it won't track the read to the derived.

To get around this, you need to read it again in the untracked context.

untrack(() => {
  this.cache.set(id, promise)
})
this.cache.get(id)

There's nothing we can really do here – if we make SvelteMap eager, then it will negatively affect it's performance. We just didn't expect people to untrack when we create signals and read them lazily!

To be clear too, const model = this.cache.get(id) is subscribing the to SvelteMap, it's subscribing to an internal signal called #version that we increment when a new property is added. However, there's another caveat here – the first time the function runs, it doesn't yet have any dependencies until after it's finished executing – that's when the runtime applies them. So when you do a mutation inside a derived of effect on the first run, we try and re-hook everything up nicely – except that won't happen if you try and make mutations inside an untrack.

Let me see if there's more we can do to work around this as it's hard for a user to work around I think

@trueadm
Copy link
Contributor

trueadm commented Oct 24, 2024

I found the solution and it's a small change to SvelteMap. PR incoming.

@gyzerok
Copy link

gyzerok commented Oct 25, 2024

@trueadm while your fix seems to fix the problem in my REPL example, unfortunately it does not seem to fix the same issue with similar code in our codebase. Not sure what the difference is yet.

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