pinia icon indicating copy to clipboard operation
pinia copied to clipboard

`$subscribe` miss mutations with type of `direct` immediately after`patch` mutations

Open duoyue opened this issue 3 years ago • 8 comments

in $subscribe ,I use console to print the mutations and states when the states change :

 store.$subscribe((mutations, state) => {
    console.log('states change :', mutations, state);
  });

it works correct when I change the state directly or use $pacth singlely

// the both ways of the follow work correct
// 1. change directly : ouput is { type:‘direct’ ,... } 
 store.count++


// 2.use $patch : output is {type: 'patch object',...}
store.$patch({
    key: `${Math.random()}`,
 });

but when I use them together as follow,the mutation with type of 'direct' will miss ,there is only {type: 'patch object',...} on console , and the count value printed was still the old value, actually had increased . seem to not be detected ?

store.$patch({
    key: `${Math.random()}`,
 });
store.count++

duoyue avatar Jan 25 '22 11:01 duoyue

This might be a caveat that cannot be worked around except by using a flush: 'sync' $subscribe() because currently, in the code, when calling $patch there is

nextTick().then(() => {
      isListening = true
    })
    isSyncListening = true
    // because we paused the watcher, we need to manually call the subscriptions
    triggerSubscriptions(
      subscriptions,
      subscriptionMutation,
      pinia.state.value[$id] as UnwrapRef<S>
    )

Moving triggerSubscriptions() to the nextTick would make this work like you want but break devtools

posva avatar Jan 25 '22 15:01 posva

This might be a caveat that cannot be worked around except by using a flush: 'sync' $subscribe() because currently, in the code, when calling $patch there is

nextTick().then(() => {
      isListening = true
    })
    isSyncListening = true
    // because we paused the watcher, we need to manually call the subscriptions
    triggerSubscriptions(
      subscriptions,
      subscriptionMutation,
      pinia.state.value[$id] as UnwrapRef<S>
    )

Moving triggerSubscriptions() to the nextTick would make this work like you want but break devtools

Thank you for your reply. Using flush:sync works, but it also changes the behavior of the watch listener . The following is the internal code of Pinia. I suspect this may be the reason why the subscription does not receive the direct mutation of the above situation . when the $patch is called, and then the watch is also triggered due to the directly change of store.count, the isListening currently has the value of false and callback wound never be called here . I don't quite understand why when the watch listener triggered, callback is not always called, but filtered using the flush: sync condition ?

 $subscribe(callback, options = {}) {
            const removeSubscription = addSubscription(subscriptions, callback, options.detached, () => stopWatcher());
            const stopWatcher = scope.run(() => watch(() => pinia.state.value[$id], (state) => {
               //  when the `$patch` is called, and then the `watch` is also triggered due to the directly change of `store.count`,
              //  the "isListening" currently has the value of `false` and `callback` wound never be called here 
                if (options.flush === 'sync' ? isSyncListening : isListening) {
                    callback({
                        storeId: $id,
                        type: MutationType.direct,
                        events: debuggerEvents,
                    }, state);
                }
            }, assign({}, $subscribeOptions, options)));
            return removeSubscription;
        }

duoyue avatar Jan 26 '22 08:01 duoyue

Because otherwise $patch would trigger subscriptions twice and using something like toRaw() to pause tracking would also remove some unwrapping.

Using $patch always trigger subscriptions in a sync fashion because it's more convenient.

Maybe splitting up the listeners by flush and making patch only trigger sync for sync listeners is a better idea but this could require a new mutation type "mixed" that would be very heavy code-wise

posva avatar Jan 26 '22 08:01 posva

subscribe mutation.events are empty in safari mobile, (I dont test safari desktop) but same code work in desktop and android mobile

hooman-mirghasemi avatar May 29 '22 08:05 hooman-mirghasemi

If I understand correctly what's going on here, the events are being debounced (similar in nature to lodash debounce). Or perhaps instead of being time sliced it is sliced based on an 'update frame'? Whatever code runs synchronously before passing back to the event loop?

In order to communicate state change correctly, the mutation data would need to be included for all mutations that are being grouped.

If Pinia mutation data is subject to race conditions and truncation, I certainly don't feel comfortable basing any of my logic on it.

For my present use case, I can code to current state and respond to that. But it puts the burden on the consumer (me) to know whether the state I'm looking at has changed or not, and whether I have previously witnessed it. Here's the data I'm receiving from $subscribe:

{
  mutation: {
    type: 'direct',
    storeId: 'request',
    events: {
      key: "class",
      newValue: null,
      oldTarget: undefined,
      target: {resource: 'Blender', class: null},
      type: 'add'
    }
  },
  state: {resource: 'Blender', class: null}
}

Snapshot of state:

{
  new_request: {resource: 'Blender', class: null}
}

Problems here are:

  1. Mutation of the resource key which occurred synchronously (and first) was truncated.
  2. mutation.events.key only includes the shallow key, not the full path which would be new_request.class, so I have no way of differentiating between different objects in the state which may have the same properties.

Guarantees like "you will receive notice of all mutations that occur" and "you will know exactly where those mutations occurred" are some of the compelling reasons for me to reach for a state management library like Pinia.

Am I understanding correctly, and is this in alignment with the goals of the project? I'm happy to submit a pull request if that would be welcome. Present mutation output could be kept for backwards compatibility and a new key introduced containing all batched mutations.

bitmage avatar Jul 19 '22 22:07 bitmage

Any way to have access to the mutation path? I have the same problem, I don't know from which object the mutation is coming.

vsambor avatar Sep 16 '22 07:09 vsambor

As a new pinia user this issue took me about two full days to find and finally have a working subscription. Thanks @posva for the sync workaround.

https://stackoverflow.com/questions/74474715/vue-3-pinia-reactive-not-updating-correctly/74485708#74485708

ItsReddi avatar Nov 18 '22 06:11 ItsReddi

@posva @bitmage @vsambor I have just run into the same issue requiring the full mutation path. I have multiple objects in the store and these objects contain the same keys. I can't tell which object the change has come from. Is there a workaround you guys know of? Cheers

peter-brennan avatar May 12 '23 21:05 peter-brennan