square / svelte-store

528 stars 36 forks source link

asyncDerived stores doesn't update when dependencies are Set or Map #68

Open hawk93 opened 1 year ago

hawk93 commented 1 year ago

Hi, in my project I had a writable store containing a Map with number as keys, that map is a dependency of an asyncDerived store, but I realized my derived store was not triggering the update. I tried to compare it with the svelte "standard" stores and it works


<script>
    import { writable, derived, asyncWritable, asyncDerived } from '@square/svelte-store';

    // Standard
    const stdSet = writable(new Set([Math.random()]));
    const stdMap = writable(new Map([[Math.random(), Math.random()]]));
    const stdDer = derived([stdSet, stdMap], ([$set, $map]) => ({ set: $set, map: $map }));

    function editStdSet() {
        stdSet.update((set) => set.add(Math.random()));
    }
    function editStdMap() {
        stdMap.update((map) => map.set(Math.random(), Math.random()));
    }

    $: console.log('standard set', $stdSet.size);
    $: console.log('standard map', $stdMap.size);
    $: console.log('standard derived', { set: $stdDer.set.size, map: $stdDer.map.size });

    // Async
    const asySet = asyncWritable([], async () => new Set([Math.random()]));
    const asyMap = asyncWritable([], async () => new Map([[Math.random(), Math.random()]]));
    const asyDer = asyncDerived([asySet, asyMap], async ([$set, $map]) => ({ set: $set, map: $map }));
    function editAsySet() {
        asySet.update((set) => set.add(Math.random()));
    }
    function editAsyMap() {
        asyMap.update((map) => map.set(Math.random(), Math.random()));
    }

    $: asySet.load().then(() => console.log('async set', $asySet.size));
    $: asyMap.load().then(() => console.log('async map', $asyMap.size));
    $: asyDer.load().then(() => console.log('async derived', { set: $asyDer.set.size, map: $asyDer.map.size }));

    // Standard writable w/ async derived
    const stdAsyDer = asyncDerived([stdSet, stdMap], async ([$set, $map]) => ({ set: $set, map: $map }));

    $: stdAsyDer.load().then(() => console.log('standard writable w/ async derived', { set: $stdAsyDer.set.size, map: $stdAsyDer.map.size }));
</script>

<p>
    <button on:click={editStdSet}>Reset Std Set</button>
    <button on:click={editStdMap}>Reset Std Map</button>
</p>

<p>
    <button on:click={editAsySet}>Reset Asy Set</button>
    <button on:click={editAsyMap}>Reset Asy Map</button>
</p>
Akolyte01 commented 1 year ago

could you reproduce this in a code sandbox please? With some comments as to the expected and actual values you are seeing? You've got a bunch of different types of stores in your code snippet, so just including what's necessary to reproduce the part that isn't working would be helpful.

hawk93 commented 1 year ago

Hi @Akolyte01 there's the reproduction in code sandbox I used a standard svelte derived store that works as expected and an asyncDerived store from the square library that is not working, just press the the button to see the problem. Thanks for your help

Akolyte01 commented 1 year ago

Thanks for that. I see the problem.... Sets and Maps are not stringifyable so the change detection gets confused. I have an idea for how to fix this but I think it might lead to some major refactoring that could take a bit. Thanks for bringing this to my attention!

hawk93 commented 1 year ago

Hi @Akolyte01, as a first fix you can try to use devalue instead of JSON.stringify(). I think that it could be a good solution waiting for a refactoring

Akolyte01 commented 1 year ago

I'm giving this rewrite a shot at the moment, but if I run into complications I'll use that for a hotfix.

jwrunge commented 1 year ago

Glad to see this issue brought up, and glad to see it is getting attention (thanks @Akolyte01!). @square/svelte-store has really made data management and reactivity MUCH more manageable in what I would call a pretty complex application I'm working on--awesome work!--but running into this issue made me wonder if I was misunderstanding how store derivation works.

Any work-arounds while @Akolyte01 resolves the issue? Just don't use Map? I'm using Maps and Sets all over a big refactor, and while they aren't the only way to go about it, they definitely help avoid duplicating data.

Akolyte01 commented 1 year ago

This is specifically a problem with Set and Map because they do not stringify properly.

JSON.stringify(new Set(['a'])) = '{}'

Stringifying is the current method used for comparing parent store values for async stores to determine if new asynchronous behavior needs to be run. This contrasts to derived stores, which just reruns the provided mapping function whenever a subscription update happens. This means that a derived store with 3 parents that each load from an initial to a final value will result in the derived store mapping function being run 6 times. This is fine when dealing with synchronous data, but would be detrimental if it resulted in hitting an endpoint 6 times as often as it needed to be.

The stringify solution is far from ideal even beyond compatibility with sets or maps because this ends up needing to be called a lot, which adds non insignificant overhead if using stores to track large data structures.

So yes, the workaround currently is to not use maps or sets. You could also fork the repo and use hawk93's suggestion to replace the JSON.stringify calls with devalue, if that would be less work for you.

However I should have a beta version of v1.1 out soon that should remove the need to stringify data as well as include some other changes like automatic rebouncing for stores.

jwrunge commented 1 year ago

Thank you. I did attempt to implement devalue and it seems like a cool package, but it doesn't like that I'm storing classes in my Maps (Cannot stringify arbitrary non-POJOs is the error message). 🤷 I guess we can't stringify everything...

For the time being, I'm removing Maps and Sets from my code and looking forward to v1.1!

jwrunge commented 1 year ago

Just wanted to jump back in and say I've been using the asyncWritable-rewrite branch and it works great! I know there is still some work to be done, but it mostly just works as intended without issue. Thanks again for all your work on this wonderful extension of Svelte store functionality!

Only thing I'm noticing is that using Store.load() may result in a store being loaded twice if it derives from another store, even if SourceStore that it's deriving from doesn't update. For example, in my startup logic, I need to send a message with the results of Store, but Store hasn't loaded yet. I use Store.load(), which first loads SourceStore, then Store and sends my message. Then Store runs its load function again, without SourceStore or itself updating.

I'm not sure if SourceStore queues a load for Store when it loads, and Store.load() is queueing that load alongside the explicit load call, or if something else is going on. If I figure out where it's coming from, I'll let you know--or if it's my bad code, I'll correct myself. It doesn't cause any major issues--just thought you should know!

Akolyte01 commented 1 year ago

Just wanted to jump back in and say I've been using the asyncWritable-rewrite branch and it works great! I know there is still some work to be done, but it mostly just works as intended without issue. Thanks again for all your work on this wonderful extension of Svelte store functionality!

Only thing I'm noticing is that using Store.load() may result in a store being loaded twice if it derives from another store, even if SourceStore that it's deriving from doesn't update. For example, in my startup logic, I need to send a message with the results of Store, but Store hasn't loaded yet. I use Store.load(), which first loads SourceStore, then Store and sends my message. Then Store runs its load function again, without SourceStore or itself updating.

I'm not sure if SourceStore queues a load for Store when it loads, and Store.load() is queueing that load alongside the explicit load call, or if something else is going on. If I figure out where it's coming from, I'll let you know--or if it's my bad code, I'll correct myself. It doesn't cause any major issues--just thought you should know!

Hey! Good to hear that things are working for you.

I think what you are witnessing is the biggest change in functionality with 1.1, and what is primarily holding back release. This change is that if a store loses all subscriptions and then gains a subscriber again, it will always run its self load functionality again. So I suspect your first load is resolving before you follow up with the other load, resulting in the two self loads. You can circumvent this (for now) by keeping a subscription active for the lifecycle that you want to interact with the store.

The reason for this ties into the change detection that's at the root of this ticket. When stringifying the parent values to see if a change has occurred, that string could continue to be tracked when the store lost its subscribers, allowing the store to know if any parent subscriptions occurred during the period the store had no subscribers. But with 1.1 this functionality was changed to instead rely on subscription updates more directly.

Consider this scenario:

  1. myAsyncDerived derives from myWritable
  2. myAsyncDerived is loaded, pulling data from myWritable and performing its self load functionality
  3. At the end of load there are no more subscriptions, so myAsyncDerived in turn unsubscribes from its parent, myWritable
  4. In this period of time myWritable may or may not change value
  5. myAsyncDerived is subscribed to, whicn in turn subscribes to myWritable and pulls its current data
  6. At this point how does myAsyncDerived know if myWritable has changed value or not, and thus whether myAsyncDerived's async functionality should be called again? We don't have signal here, so thus far my implementation has been to just run the async functionality again.

The only way I have thought of to get around this is for async stores to save their parent values when it goes inactive (due to not having any subscribers) and then when it goes active again it can compare the current parent values to the stored values to determine if it needs to load itself again or use the current value of the store.

But the problem with this is that this comparison would need to use stringify or equivalent, which just reintroduces the original problem outlined by this ticket.

jwrunge commented 1 year ago

Admittedly, I haven't dug into the code (I'll have to take a look!), so I don't know to what degree you are rewriting svelte/store or what you have access to.

Off the top of my head, I'm thinking that if a store knows that it should broadcast its own new value, it can store the current timestamp, and when the child updates it can store that same timestamp in a parentTimestamp member. When the child "goes active" and would normally check against the parent's value, it can just compare when it thinks the parent was last updated to when the parent actually DID update itself. That way, you could avoid any sort of expensive comparisons and just rely on the parent's own knowledge of the last time it changed.

That all may be totally not how derived stores work, so if I get some time this week, I'll dig into the code and see if I get any bright ideas.