fix(subscribe): avoid $subscriptions with $patch

posva · posva · commit 3bfe9e51fd0f · 2021-12-20T20:08:19.000+01:00
Fix #908
diff --git a/packages/docs/core-concepts/state.md b/packages/docs/core-concepts/state.md
@@ -151,15 +151,15 @@ cartStore.$subscribe((mutation, state) => {
 })
 ```
 
-By default, _state subscriptions_ are bound to the component where they are added (if the store is inside a component's `setup()`). Meaning, they will be automatically removed when the component is unmounted. If you want to keep them after the component is unmounted, pass `true` as the second argument to _detach_ the _state subscription_ from the current component:
+By default, _state subscriptions_ are bound to the component where they are added (if the store is inside a component's `setup()`). Meaning, they will be automatically removed when the component is unmounted. If you want to keep them after the component is unmounted, pass `{ detached: true }` as the second argument to _detach_ the _state subscription_ from the current component:
 
 ```js
 export default {
   setup() {
     const someStore = useSomeStore()
 
     // this subscription will be kept after the component is unmounted
-    someStore.$subscribe(callback, true)
+    someStore.$subscribe(callback, { detached: true })
 
     // ...
   },
diff --git a/packages/pinia/__tests__/subscriptions.spec.ts b/packages/pinia/__tests__/subscriptions.spec.ts
@@ -37,6 +37,7 @@ describe('Subscriptions', () => {
     const patch = { user: 'Cleiton' }
     store.$patch(patch)
 
+    expect(spy).toHaveBeenCalledTimes(1)
     expect(spy).toHaveBeenCalledWith(
       expect.objectContaining({
         payload: patch,
@@ -46,6 +47,116 @@ describe('Subscriptions', () => {
       store.$state
     )
   })
+  const flushOptions = ['post', 'pre', 'sync'] as const
+
+  flushOptions.forEach((flush) => {
+    it('calls once inside components with flush ' + flush, async () => {
+      const pinia = createPinia()
+      setActivePinia(pinia)
+      const spy1 = jest.fn()
+
+      mount(
+        {
+          setup() {
+            const s1 = useStore()
+            s1.$subscribe(spy1, { flush })
+          },
+          template: `<p/>`,
+        },
+        { global: { plugins: [pinia] } }
+      )
+
+      const s1 = useStore()
+
+      expect(spy1).toHaveBeenCalledTimes(0)
+
+      s1.user = 'Edu'
+      await nextTick()
+      await nextTick()
+      expect(spy1).toHaveBeenCalledTimes(1)
+
+      s1.$patch({ user: 'a' })
+      await nextTick()
+      await nextTick()
+      expect(spy1).toHaveBeenCalledTimes(2)
+
+      s1.$patch((state) => {
+        state.user = 'other'
+      })
+      await nextTick()
+      await nextTick()
+      expect(spy1).toHaveBeenCalledTimes(3)
+    })
+  })
+
+  it('works with multiple different flush', async () => {
+    const spyPre = jest.fn()
+    const spyPost = jest.fn()
+    const spySync = jest.fn()
+
+    const s1 = useStore()
+    s1.$subscribe(spyPre, { flush: 'pre' })
+    s1.$subscribe(spyPost, { flush: 'post' })
+    s1.$subscribe(spySync, { flush: 'sync' })
+
+    expect(spyPre).toHaveBeenCalledTimes(0)
+    expect(spyPost).toHaveBeenCalledTimes(0)
+    expect(spySync).toHaveBeenCalledTimes(0)
+
+    s1.user = 'Edu'
+    expect(spyPre).toHaveBeenCalledTimes(0)
+    expect(spyPost).toHaveBeenCalledTimes(0)
+    expect(spySync).toHaveBeenCalledTimes(1)
+    await nextTick()
+    expect(spyPre).toHaveBeenCalledTimes(1)
+    expect(spyPost).toHaveBeenCalledTimes(1)
+    expect(spySync).toHaveBeenCalledTimes(1)
+
+    s1.$patch({ user: 'a' })
+    // patch still triggers all subscriptions immediately
+    expect(spyPre).toHaveBeenCalledTimes(2)
+    expect(spyPost).toHaveBeenCalledTimes(2)
+    expect(spySync).toHaveBeenCalledTimes(2)
+    await nextTick()
+    expect(spyPre).toHaveBeenCalledTimes(2)
+    expect(spyPost).toHaveBeenCalledTimes(2)
+    expect(spySync).toHaveBeenCalledTimes(2)
+
+    s1.$patch((state) => {
+      state.user = 'other'
+    })
+    expect(spyPre).toHaveBeenCalledTimes(3)
+    expect(spyPost).toHaveBeenCalledTimes(3)
+    expect(spySync).toHaveBeenCalledTimes(3)
+    await nextTick()
+    expect(spyPre).toHaveBeenCalledTimes(3)
+    expect(spyPost).toHaveBeenCalledTimes(3)
+    expect(spySync).toHaveBeenCalledTimes(3)
+  })
+
+  it('works with multiple different flush and multiple state changes', async () => {
+    const spyPre = jest.fn()
+    const spyPost = jest.fn()
+    const spySync = jest.fn()
+
+    const s1 = useStore()
+    s1.$subscribe(spyPre, { flush: 'pre' })
+    s1.$subscribe(spyPost, { flush: 'post' })
+    s1.$subscribe(spySync, { flush: 'sync' })
+
+    s1.user = 'Edu'
+    expect(spyPre).toHaveBeenCalledTimes(0)
+    expect(spyPost).toHaveBeenCalledTimes(0)
+    expect(spySync).toHaveBeenCalledTimes(1)
+    s1.$patch({ user: 'a' })
+    expect(spyPre).toHaveBeenCalledTimes(1)
+    expect(spyPost).toHaveBeenCalledTimes(1)
+    expect(spySync).toHaveBeenCalledTimes(2)
+    await nextTick()
+    expect(spyPre).toHaveBeenCalledTimes(1)
+    expect(spyPost).toHaveBeenCalledTimes(1)
+    expect(spySync).toHaveBeenCalledTimes(2)
+  })
 
   it('unsubscribes callback when unsubscribe is called', () => {
     const spy = jest.fn()
diff --git a/packages/pinia/src/store.ts b/packages/pinia/src/store.ts
@@ -20,6 +20,7 @@ import {
   ref,
   set,
   del,
+  nextTick,
   isVue2,
 } from 'vue-demi'
 import {
@@ -229,6 +230,7 @@ function createSetupStore<
 
   // internal state
   let isListening: boolean // set to true at the end
+  let isSyncListening: boolean // set to true at the end
   let subscriptions: SubscriptionCallback<S>[] = markRaw([])
   let actionSubscriptions: StoreOnActionListener<Id, S, G, A>[] = markRaw([])
   let debuggerEvents: DebuggerEvent[] | DebuggerEvent
@@ -255,7 +257,7 @@ function createSetupStore<
       | ((state: UnwrapRef<S>) => void)
   ): void {
     let subscriptionMutation: SubscriptionCallbackMutation<S>
-    isListening = false
+    isListening = isSyncListening = false
     // reset the debugger events since patches are sync
     /* istanbul ignore else */
     if (__DEV__) {
@@ -277,7 +279,10 @@ function createSetupStore<
         events: debuggerEvents as DebuggerEvent[],
       }
     }
-    isListening = true
+    nextTick().then(() => {
+      isListening = true
+    })
+    isSyncListening = true
     // because we paused the watcher, we need to manually call the subscriptions
     triggerSubscriptions(
       subscriptions,
@@ -384,7 +389,7 @@ function createSetupStore<
         watch(
           () => pinia.state.value[$id] as UnwrapRef<S>,
           (state) => {
-            if (isListening) {
+            if (options.flush === 'sync' ? isSyncListening : isListening) {
               callback(
                 {
                   storeId: $id,
@@ -575,8 +580,12 @@ function createSetupStore<
 
       // avoid devtools logging this as a mutation
       isListening = false
+      isSyncListening = false
       pinia.state.value[$id] = toRef(newStore._hmrPayload, 'hotState')
-      isListening = true
+      isSyncListening = true
+      nextTick().then(() => {
+        isListening = true
+      })
 
       for (const actionName in newStore._hmrPayload.actions) {
         const action: _Method = newStore[actionName]
@@ -702,6 +711,7 @@ function createSetupStore<
   }
 
   isListening = true
+  isSyncListening = true
   return store
 }
 
diff --git a/packages/pinia/src/types.ts b/packages/pinia/src/types.ts
@@ -349,15 +349,13 @@ export interface _StoreWithState<
   $reset(): void
 
   /**
-   * Setups a callback to be called whenever the state changes. It also returns
-   * a function to remove the callback. Note than when calling
-   * `store.$subscribe()` inside of a component, it will be automatically
-   * cleanup up when the component gets unmounted unless `detached` is set to
-   * true.
+   * Setups a callback to be called whenever the state changes. It also returns a function to remove the callback. Note
+   * than when calling `store.$subscribe()` inside of a component, it will be automatically cleanup up when the
+   * component gets unmounted unless `detached` is set to true.
    *
    * @param callback - callback passed to the watcher
-   * @param options - `watch` options + `detached` to detach the subscription
-   * from the context (usually a component) this is called from
+   * @param options - `watch` options + `detached` to detach the subscription from the context (usually a component)
+   * this is called from. Note that the `flush` option does not affect calls to `store.$patch()`.
    * @returns function that removes the watcher
    */
   $subscribe(
