From 4dc1f1bec8f3db508c02fa6bdcc8203f280cbb3e Mon Sep 17 00:00:00 2001 From: Eduardo San Martin Morote Date: Mon, 13 Dec 2021 11:06:43 +0100 Subject: [PATCH] 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. --- packages/pinia/__tests__/actions.spec.ts | 10 +++---- packages/pinia/__tests__/onAction.spec.ts | 15 +++++++++++ packages/pinia/src/store.ts | 33 +++++++++++------------ 3 files changed, 36 insertions(+), 22 deletions(-) diff --git a/packages/pinia/__tests__/actions.spec.ts b/packages/pinia/__tests__/actions.spec.ts index 36d7e2c7..0fbc0b20 100644 --- 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 7d054303..354177f1 100644 --- 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 f3682d86..b5894384 100644 --- 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 extends Array ? T : never + function mergeReactiveObjects( target: T, patchToApply: DeepPartial @@ -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) { + afterCallbackList.push(callback) } - function onError(callback: typeof onErrorCallback) { - onErrorCallback = callback + function onError(callback: _ArrayType) { + 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 } } -- 2.39.5