svelte icon indicating copy to clipboard operation
svelte copied to clipboard

Store is not being updated synchronously / store value is incorrect

Open yuliankarapetkov opened this issue 4 years ago • 17 comments

Describe the bug Store value is not up-to-date if using a $ subscription inside a subscription function.

To Reproduce

  1. Open this REPL.
  2. Open your browser console.
  3. Click on "Increment" a few times.
  4. Click on "Cause reset".
  5. Check your console.

For a real life example look at this REPL.

image

Expected behavior Value should be in sync.

Severity Blocking

yuliankarapetkov avatar Jun 22 '20 16:06 yuliankarapetkov

Do you have a reproduction with fewer moving parts? Are the spreads an important part of this? Is the text input?

Conduitry avatar Jun 22 '20 21:06 Conduitry

Do you have a reproduction with fewer moving parts? Are the spreads an important part of this? Is the text input?

The text input is not important, you can just do the following:

  1. Click on "Increment page" a few times.
  2. Click on "Filter".

Then, the result would be the same.

Update: I updated my original post with a shorter REPL that's probably easier to understand.

yuliankarapetkov avatar Jun 23 '20 08:06 yuliankarapetkov

I'm going to disagree that it's a blocker because replacing line 12 with: $count = 0 fixes the issue.

However I'm going to agree that it looks like a bug, for the reason that:

count.set(0)
$count = 0 

should have the same behaviour, I believe.

antony avatar Jun 23 '20 08:06 antony

I'm going to disagree that it's a blocker because replacing line 12 with: $count = 0 fixes the issue.

However I'm going to agree that it looks like a bug, for the reason that:

count.set(0)
$count = 0 

should have the same behaviour, I believe.

And it's also not working correctly for count.update(v => 0) as well, which means that for updates you have to use this syntax instead:

$store = { ...$store, page: 0 }

yuliankarapetkov avatar Jun 23 '20 08:06 yuliankarapetkov

AFAIK in the docs it says store.set(value) is the proper way to set a store's value, so I would say it's a bug that it doesn't work as described.

And if $count = 0 works, but count.set(0) doesn't, then why do we even have the function syntax? 😕 We should just use the assignment syntax everywhere instead.

jhwheeler avatar Jun 23 '20 09:06 jhwheeler

It does work, but it is async.

I think $count = 0; has something like a "auto await tick", because if you change the reset function like this

import { tick } from 'svelte';
async function reset () {
	count.set(0)
	await tick
	console.log('count should be zero ', $count)
}

it works.

PatrickG avatar Jun 23 '20 12:06 PatrickG

svelte store sets are always synchronous

in your examples, the second store is not a store you're only using the event dispatching feature of the store

the issue is that stores updated by other stores are assumed to be derived stores, so svelte treats them differently

$count = 0 does work because it's known "for sure" that $count will be set to 0, so it sets the value locally at the same time as it calls count.set

set_store_value(count, $count = 0);

that however is willy-nilly compiler optimization that should be removed, as that causes stores such as tweened and spring to be temporally out of sync with the local component value and it only works locally as $count will still be out of sync in other components

the value should be set synchronously anyway so setting it locally at set_store_value before it gets set in component_subscribe is just unnecessary extra work that only hides bad uses of stores such as this one

pushkine avatar Jun 23 '20 12:06 pushkine

$count = 0 does work because it's known "for sure" that $count will be set to 0, so it sets the value locally at the same time as it calls count.set

OK, that's interesting. Why do we use store.set(value) at all, instead of just assigning stores to their new values like we do with component-scope variables?

jhwheeler avatar Jun 23 '20 13:06 jhwheeler

in your examples, the second store is not a store you're only using the event dispatching feature of the store

In fact, in my real-world example the second store is a store.

that only hides bad uses of stores such as this one

I need the filter to be a store because it is part of the app state. So, if filter is a store anyway and I need to perform an operation when its value changes, how is it a "bad use" to perform this operation in the subscribe function? 🤔

Anyway, the point of this thread is to report an obvious bug and not to discuss code quality.

yuliankarapetkov avatar Jun 23 '20 15:06 yuliankarapetkov

what if we use a reactive block instead of subscription function, like this...

$: if ($random) { reset(); }

this feels bit more svelte/reactive... isn't this same as the subscription function.. mixing sync and async is bad as usual, either it should just work out of box "somehow" Or there should be a lint and compiler error for calling sync functions (for those which have async ways using them) within async function flows... just a thought.. (it's good i saw this, I'm pretty sure i will waste hours on something like this, try to debug)

Dulanjala007 avatar Jun 25 '20 15:06 Dulanjala007

@jhwheeler as pushkine said, when you use assignment Svelte assumes that assignment is what you want and sets the store value immediately, regardless of what then happens in set(). Calling set() directly avoids this behavior (might be desirable for custom stores), though frankly I think you'd usually be better off naming the method something else and avoiding the confusion of it also being called after assignment.

I was confused enough by this to ask about it on Stackoverflow. I agree with antony; I would have expected set() and assignment to have the same behavior by default.

jwlarocque avatar Jul 22 '20 15:07 jwlarocque

@jhwheeler as pushkine said, when you use assignment Svelte assumes that assignment is what you want and sets the store value immediately, regardless of what then happens in set(). Calling set() directly avoids this behavior (might be desirable for custom stores), though frankly I think you'd usually be better off naming the method something else and avoiding the confusion of it also being called after assignment.

I was confused enough by this to ask about it on Stackoverflow. I agree with antony; I would have expected set() and assignment to have the same behavior by default.

I've just stumbled upon this issue while experimenting with animations. I had my two client sizes (clientWidth and clientHeight) bound to an element and was using their value in a tweened store to animate a different component.

My setup and the assignment using the reactive operator caused the value to update immediately before tweening - like @jwlarocque noted. Here's an example with the red square jumping before animating: https://svelte.dev/repl/bcc79762e77443f5b9a5bff26c49a57e?version=3.25.0

Here's an example with the store's set method, instead (no jumping): https://svelte.dev/repl/ce5c48faabec4a1aa771e2c65870d841?version=3.25.0

It would help a lot for beginners if the two behaviours were equivalent (I personally find the assignment's behaviour counter-intuitive) or if there was at least a note in the documentation highlighting this difference.

IgnusG avatar Sep 11 '20 23:09 IgnusG

I've read through this thread a few times, and while I'm not quite sure I understand everything, I'd like to note for the next person who sees this, that if you want to synchronously see the new value reflected,

get(count)

does the trick

console.log('count should be zero ', $count, get(count));

and prints

count should be zero 5 0

https://svelte.dev/repl/be858d84e6364019855d4fb85e2e4c53?version=3.23.2

arackaf avatar Oct 28 '20 02:10 arackaf

What's especially encouraging is that derived stores do in fact synchronously reflect their new values when the source stores upstream are set with set no matter whether they're accessed via $derivedStore or via get(derivedStore)

https://svelte.dev/repl/2dca058dd63d4ea39597bc0de6a9bc0e?version=3.23.2

arackaf avatar Oct 28 '20 02:10 arackaf

First of all, store.set(value) is synchronous.

if you call store.set(5), you should be able to read the value out of the store immediately, get(store) === 5.

Secondly, the magic of $store is:

let $store;
store.subscribe(store, (value) => $store = value);

$store is another variable that stays in-sync with the store value by subscribing to it

in most cases, the subscribe callback function is called synchronously, so you can do:

store.set(5);
console.log($store); // 5

so what happened in the above code is that:

  1. store.set(5) will synchronously loop through the subscribers function and call them
  2. (value) => $store = value) is evaluated, therefore the value of $store updated to 5
  3. after all the subscribers are looped through, the store.set(...) method returns, and ...
  4. console.log($store) prints out 5

allow me to break down what happen instead, if you write $store = 5, as some of you are confused and wonder what's the difference between store.set(...) and $store = ...

when you write $store = 5 in a .svelte component, it is compiled into:

store.set($store = 5);
  1. $store = 5 updates the variable $store to 5
  2. the expression $store = 5 returns 5, therefore store.set(5)
  3. store.set(5) will synchronously loop through the subscribers function and call them
  4. (value) => $store = value) is evaluated, therefore the value of $store updated to 5, though at this point, it is already 5
  5. after all the subscribers are looped through, the store.set(...) method returns

strictly speaking, in this case, the value of $store gets update, even before value of the store gets update, but it all happen within the same statement, it is unlikely to have a race condition.


Now, if we are all aligned with the fundamentals of store, here is what introduced this bug:

https://github.com/sveltejs/svelte/pull/3219

which introduced an optimisation to update store value via breadth-first approach, vs depth-first approach, as explained in https://github.com/sveltejs/svelte/pull/3219#issuecomment-515444121

how would that impact in our case? let's take a look at the following example:

let store = writable(5);
store.subscribe(() => {
	store.set(10)
	console.log($store);
});
store.set(30);

repl

we subscribe the store and call store.set() in the subscriber function to update the store value to 10, (it wont lead to infinite loop, because internally, writable will not notify the subscribers if the value is set to the same value), so you set it to 30, it will set it to 10 and then it will try to set it to 10 again, which will be a noop.

guess what is the value of $store inside store.subscribe(...) ? you'll see the function being called a few times, but you'll find 30 is printed among them!

so, if the store is updated in depth-first approach, whenever you call store.set(...) it will call the subscriber callback function immediately, and update the value of the $store:

  1. store.set(30)
    1. calls (value) => $store = value to update the $store to 30
    2. calls () => { ... }
    3. which calls store.set(10)
      1. calls (value) => $store = value to update the $store to 10
      2. calls () => { ... }
      3. which calls store.set(10)
        1. at this point of time, the value of the store is already 10, so it is a noop.
      4. console.log($store) prints out 10
    4. console.log($store) prints out 10

Run the above code in v3.6.8 which was before the optimisation got introduced, and you'll see the above REPL

However, now it is run in breadth-first approach:

  1. store.set(30)
    1. calls (value) => $store = value to update the $store to 30
    2. calls () => { ... }
    3. which calls store.set(10)
      1. schedules (value) => $store = value
      2. schedules () => { ... }
    4. console.log($store) prints out 30
    5. calls the scheduled (value) => $store = value to update the $store to 10
    6. calls the scheduled () => {...}
      1. which calls store.set(10)
        1. at this point of time, the value of the store is already 10, so it is a noop.
      2. console.log($store) prints out 10

now see that the inner store.set(10) schedules the update of the variable $store, therefore you see $store = 30?

that's exactly what happened in this issue:

random.subscribe(v => reset())

const updateRandom = () => random.set(Date.now())

function reset () {
	count.set(0)
	console.log('count should be zero ', $count)
}

calling random.set(), which in the subscribe function calls count.set(0) -> causes the (value) => $count = value get scheduled, therefore printing the value out immediately still show the old $count value.

but if you do:

const updateRandom = () => {
  random.set(Date.now())
  console.log('count is zero', $count);
};

you'll see 0, since random.set() started the store update chain, when it finishes, the value of $random and $count should already been updated.

so, what then?


Workaround / Solution

Use reactive declaration:

$: $random, reset();

tanhauhau avatar Jun 22 '21 11:06 tanhauhau

I am running into this and having to unwind a ton of code to try and exactly identify where the problem(s) originates. I typically used $store over store.set but had to use store.set when using in my typescript portions. Was the optimization gain from https://github.com/sveltejs/svelte/pull/3219 worth the side-effect?

Jojoshua avatar Oct 02 '22 20:10 Jojoshua

I was chasing down a component which wasn't updating, based on some store data being propagated down from parent components. I thought it was some sync/async problem with stores, or props not being changed properly, but it turns out this reactivity fix above was the way to solve it.

I had an object passed in as a prop called property and a display value I was wanting to compute, so in my script block I changed it to:

let value: string;

$: (value = getValue()), property;

I assume this tells Svelte "any time that 'property' object changes, run this reactive function" - which would update my display value.

achamas-playco avatar Dec 01 '22 09:12 achamas-playco

@Conduitry Do we expect this to be fixed with Svelte 5?

jhwheeler avatar Sep 21 '23 09:09 jhwheeler

@Conduitry Do we expect this to be fixed with Svelte 5?

It should be when using the new rune mode

IgnusG avatar Sep 21 '23 13:09 IgnusG