fix: allow using multiple `$onAction`, **ignore returned value**

Fix #893

This change removes the ability to return `false` to avoid an action to
throw because differently from `onErrorCaptured` from Vue 3, errors are
not propagated here and this behavior could lead to actions not throwing
inside components, which is is rather error-prone. This behavior can
still be achieved with a plugin that override every action in a store.
It also removes the ability to override the returned value with `after`,
which was error prone for the same arguments. Fortunately, these were
**undocumented features**, and should not be used. Revisitting the old
implementation could be revisited but should go through an RFC to
discuss different possibilities (e.g. should the returned value inside
`after` be passed to any other `after`?) to find the most one that makes
more sense for a default.

diff --git a/packages/pinia/__tests__/actions.spec.ts b/packages/pinia/__tests__/actions.spec.ts
index 36d7e2c775425d9ba2fdf3fdb2a39b9e02c5a47d..0fbc0b20116101d750e06c94d37750a89153cca6 100644 (file)
--- a/packages/pinia/__tests__/actions.spec.ts
+++ b/packages/pinia/__tests__/actions.spec.ts
@@ -147,7 +147,7 @@ describe('Actions', () => {
     expect(() => store.throws()).toThrowError('fail')
   })
 
-  it('can avoid errors to propagate', () => {
+  it.skip('can avoid thrown errors to propagate', () => {
     const store = useStore()
     store.$onAction(({ onError }) => {
       onError(() => false)
@@ -155,7 +155,7 @@ describe('Actions', () => {
     expect(() => store.throws()).not.toThrowError('fail')
   })
 
-  it('can avoid async errors to propagate', async () => {
+  it.skip('can avoid async errors to propagate', async () => {
     const store = useStore()
     store.$onAction(({ onError }) => {
       onError(() => false)
@@ -178,7 +178,7 @@ describe('Actions', () => {
     expect(spy).toHaveBeenCalledWith('fail')
   })
 
-  it('can override the returned value', () => {
+  it.skip('can override the returned value', () => {
     const store = useStore()
     expect.assertions(2)
     store.$onAction(({ after }) => {
@@ -191,7 +191,7 @@ describe('Actions', () => {
     expect(store.toggle()).toBe('hello')
   })
 
-  it('can override the resolved value', async () => {
+  it.skip('can override the resolved value', async () => {
     const store = useStore()
     expect.assertions(2)
     store.$onAction(({ after }) => {
@@ -204,7 +204,7 @@ describe('Actions', () => {
     await expect(store.getNonA()).resolves.toBe('hello')
   })
 
-  it('can override the resolved value with a promise', async () => {
+  it.skip('can override the resolved value with a promise', async () => {
     const store = useStore()
     expect.assertions(2)
     store.$onAction(({ after }) => {

diff --git a/packages/pinia/__tests__/onAction.spec.ts b/packages/pinia/__tests__/onAction.spec.ts
index 7d054303cd6f03046b0cd82f9784d11bd95f2ad2..354177f1d6ac51b8727097af195fcafb171b8a82 100644 (file)
--- a/packages/pinia/__tests__/onAction.spec.ts
+++ b/packages/pinia/__tests__/onAction.spec.ts
@@ -66,6 +66,21 @@ describe('Subscriptions', () => {
     expect(spy).not.toHaveBeenCalled()
   })
 
+  it('can register multiple onAction', async () => {
+    const spy1 = jest.fn()
+    const spy2 = jest.fn()
+    store.$onAction(({ after }) => {
+      after(spy1)
+    })
+    store.$onAction(({ after }) => {
+      after(spy2)
+    })
+
+    await expect(store.asyncUpperName()).resolves.toBe('EDUARDO')
+    expect(spy2).toHaveBeenCalledTimes(1)
+    expect(spy1).toHaveBeenCalledTimes(1)
+  })
+
   it('calls after with the returned value', async () => {
     const spy = jest.fn()
     // Cannot destructure because of https://github.com/microsoft/TypeScript/issues/38020

diff --git a/packages/pinia/src/store.ts b/packages/pinia/src/store.ts
index f3682d86fc7cc35b2ba50a01a454e438b2539479..b589438491185b21d1cb6b040c3bf9e9fa00bbac 100644 (file)
--- a/packages/pinia/src/store.ts
+++ b/packages/pinia/src/store.ts
@@ -50,6 +50,8 @@ import { IS_CLIENT } from './env'
 import { patchObject } from './hmr'
 import { addSubscription, triggerSubscriptions } from './subscriptions'
 
+type _ArrayType<AT> = AT extends Array<infer T> ? T : never
+
 function mergeReactiveObjects<T extends StateTree>(
   target: T,
   patchToApply: DeepPartial<T>
@@ -314,13 +316,13 @@ function createSetupStore<
       setActivePinia(pinia)
       const args = Array.from(arguments)
 
-      let afterCallback: (resolvedReturn: any) => any = noop
-      let onErrorCallback: (error: unknown) => unknown = noop
-      function after(callback: typeof afterCallback) {
-        afterCallback = callback
+      const afterCallbackList: Array<(resolvedReturn: any) => any> = []
+      const onErrorCallbackList: Array<(error: unknown) => unknown> = []
+      function after(callback: _ArrayType<typeof afterCallbackList>) {
+        afterCallbackList.push(callback)
       }
-      function onError(callback: typeof onErrorCallback) {
-        onErrorCallback = callback
+      function onError(callback: _ArrayType<typeof onErrorCallbackList>) {
+        onErrorCallbackList.push(callback)
       }
 
       // @ts-expect-error
@@ -337,28 +339,25 @@ function createSetupStore<
         ret = action.apply(this && this.$id === $id ? this : store, args)
         // handle sync errors
       } catch (error) {
-        if (onErrorCallback(error) !== false) {
-          throw error
-        }
+        triggerSubscriptions(onErrorCallbackList, error)
+        throw error
       }
 
       if (ret instanceof Promise) {
         return ret
           .then((value) => {
-            const newRet = afterCallback(value)
-            // allow the afterCallback to override the return value
-            return newRet === undefined ? value : newRet
+            triggerSubscriptions(afterCallbackList, value)
+            return value
           })
           .catch((error) => {
-            if (onErrorCallback(error) !== false) {
-              return Promise.reject(error)
-            }
+            triggerSubscriptions(onErrorCallbackList, error)
+            return Promise.reject(error)
           })
       }
 
       // allow the afterCallback to override the return value
-      const newRet = afterCallback(ret)
-      return newRet === undefined ? ret : newRet
+      triggerSubscriptions(afterCallbackList, ret)
+      return ret
     }
   }