fix(subscribe): direct mutation should not trigger detached subscriptions

Fix #908

Author: posva

packages/pinia/__tests__/subscriptions.spec.ts

@@ -124,20 +124,33 @@ describe('Subscriptions', () => {
       expect(spy2).toHaveBeenCalledTimes(0)
 
       s1.name = 'Edu'
-
       expect(spy1).toHaveBeenCalledTimes(1)
       expect(spy2).toHaveBeenCalledTimes(1)
 
       s1.$patch({ name: 'a' })
       expect(spy1).toHaveBeenCalledTimes(2)
       expect(spy2).toHaveBeenCalledTimes(2)
 
+      s1.$patch((state) => {
+        state.name = 'other'
+      })
+      expect(spy1).toHaveBeenCalledTimes(3)
+      expect(spy2).toHaveBeenCalledTimes(3)
+
       wrapper.unmount()
       await nextTick()
 
       s1.$patch({ name: 'b' })
-      expect(spy1).toHaveBeenCalledTimes(2)
-      expect(spy2).toHaveBeenCalledTimes(3)
+      expect(spy1).toHaveBeenCalledTimes(3)
+      expect(spy2).toHaveBeenCalledTimes(4)
+      s1.$patch((state) => {
+        state.name = 'c'
+      })
+      expect(spy1).toHaveBeenCalledTimes(3)
+      expect(spy2).toHaveBeenCalledTimes(5)
+      s1.name = 'd'
+      expect(spy1).toHaveBeenCalledTimes(3)
+      expect(spy2).toHaveBeenCalledTimes(6)
     })
   })
 

packages/pinia/src/store.ts

@@ -48,7 +48,7 @@ import {
 import { setActivePinia, piniaSymbol, Pinia, activePinia } from './rootStore'
 import { IS_CLIENT } from './env'
 import { patchObject } from './hmr'
-import { addSubscription, triggerSubscriptions } from './subscriptions'
+import { addSubscription, triggerSubscriptions, noop } from './subscriptions'
 
 type _ArrayType<AT> = AT extends Array<infer T> ? T : never
 
@@ -173,8 +173,6 @@ function createOptionsStore<
   return store as any
 }
 
-const noop = () => {}
-
 function createSetupStore<
   Id extends string,
   SS,
@@ -376,10 +374,11 @@ function createSetupStore<
     $patch,
     $reset,
     $subscribe(callback, options = {}) {
-      const _removeSubscription = addSubscription(
+      const removeSubscription = addSubscription(
         subscriptions,
         callback,
-        options.detached
+        options.detached,
+        () => stopWatcher()
       )
       const stopWatcher = scope.run(() =>
         watch(
@@ -400,11 +399,6 @@ function createSetupStore<
         )
       )!
 
-      const removeSubscription = () => {
-        stopWatcher()
-        _removeSubscription()
-      }
-
       return removeSubscription
     },
     $dispose,

packages/pinia/src/subscriptions.ts

@@ -1,17 +1,21 @@
 import { getCurrentInstance, onUnmounted } from 'vue-demi'
 import { _Method } from './types'
 
+export const noop = () => {}
+
 export function addSubscription<T extends _Method>(
   subscriptions: T[],
   callback: T,
-  detached?: boolean
+  detached?: boolean,
+  onCleanup: () => void = noop
 ) {
   subscriptions.push(callback)
 
   const removeSubscription = () => {
     const idx = subscriptions.indexOf(callback)
     if (idx > -1) {
       subscriptions.splice(idx, 1)
+      onCleanup()
     }
   }
 

packages/pinia/src/types.ts

@@ -333,11 +333,11 @@ export interface _StoreWithState<
   /**
    * Group multiple changes into one function. Useful when mutating objects like
    * Sets or arrays and applying an object patch isn't practical, e.g. appending
-   * to an array.
+   * to an array. The function passed to `$patch()` **must be synchronous**.
    *
    * @param stateMutator - function that mutates `state`, cannot be async
    */
-  $patch<F extends (state: UnwrapRef<S>) => void>(
+  $patch<F extends (state: UnwrapRef<S>) => any>(
     // this prevents the user from using `async` which isn't allowed
     stateMutator: ReturnType<F> extends Promise<any> ? never : F
   ): void

packages/pinia/test-dts/state.test-d.ts

@@ -53,6 +53,11 @@ const useStore = defineStore({
 
 const store = useStore()
 
+store.$patch({ counter: 2 })
+store.$patch((state) => {
+  expectType<number>(state.counter)
+})
+
 expectType<number>(store.$state.counter)
 expectType<number>(store.$state.double)
 
@@ -62,3 +67,15 @@ expectType<number>(store.fromARef)
 
 expectType<{ msg: string }>(store.aShallowRef)
 expectType<{ msg: string }>(store.$state.aShallowRef)
+
+const onlyState = defineStore({
+  id: 'main',
+  state: () => ({
+    counter: 0,
+  }),
+})()
+
+onlyState.$patch({ counter: 2 })
+onlyState.$patch((state) => {
+  expectType<number>(state.counter)
+})