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

$subscribe miss mutations with type of direct immediately afterpatch mutations #992

Open
duoyue opened this issue Jan 25, 2022 · 8 comments
Labels
discussion has workaround The issue contains a temporary solution to get around the problem

Comments

@duoyue
Copy link

duoyue commented Jan 25, 2022

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++
@posva
Copy link
Member

posva commented Jan 25, 2022

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 posva added the has workaround The issue contains a temporary solution to get around the problem label Jan 25, 2022
@duoyue
Copy link
Author

duoyue commented Jan 26, 2022

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;
        }

@posva
Copy link
Member

posva commented Jan 26, 2022

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

@hooman-mirghasemi
Copy link

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

@bitmage
Copy link

bitmage commented Jul 19, 2022

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.

@vsambor
Copy link

vsambor commented Sep 16, 2022

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.

@ItsReddi
Copy link

ItsReddi commented Nov 18, 2022

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

@peter-brennan
Copy link

peter-brennan commented May 12, 2023

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion has workaround The issue contains a temporary solution to get around the problem
Projects
None yet
Development

No branches or pull requests

7 participants