Repository: ctrlplusb/easy-peasy
Pull request #860 (2023-08-11 10:03:35)

fix: set default error on empty helpers.fail() call.
## Issue

The current implementation of the `helpers.fail()` method triggers a thunk fail only when an error argument is provided. This behavior can be misleading given the documentation states:

*You may pass your error object or any arbitrary payload to fail.*

However, it does not explicitly mention that passing an error is compulsory.

## Proposed Solutions
Three potential ways to address this ambiguity include:

1. Documentation Update: Elaborate in the documentation that the thunk will NOT fail if the method is invoked without parameters.
2. TypeScript Change (Breaking Change): Modify the TypeScript definition of the fail method to require a parameter, ensuring users always provide one. This however wont affect the JS users.
3. Behavior Modification (This PR): Adjust the method to fail a thunk irrespective of parameter presence. In cases without a parameter, default the error value to an empty Error object. (Note: This might introduce unforeseen side-effects, for example users relying on a success thunk after having called an empty helpers.fail().)






vercel[bot]: [vc]: #xYP4QwvfR/RT187W5s5JfQULVgO2rCeXq1fQ6uqL3mw=:eyJpc01vbm9yZXBvIjp0cnVlLCJ0eXBlIjoiZ2l0aHViIiwicHJvamVjdHMiOlt7Im5hbWUiOiJlYXN5LXBlYXN5Iiwicm9vdERpcmVjdG9yeSI6IndlYnNpdGUiLCJpbnNwZWN0b3JVcmwiOiJodHRwczovL3ZlcmNlbC5jb20vY3RybHBsdXNiL2Vhc3ktcGVhc3kvOVJMdWhmMmR4Z2JOZkU4Mm5kdnBEdEpZcXpFdSIsInByZXZpZXdVcmwiOiJlYXN5LXBlYXN5LWdpdC1mb3JrLXNjYXNjYXItZml4LWZhaWwtdGh1bmstb24tMmQ3NmNlLWN0cmxwbHVzYi52ZXJjZWwuYXBwIiwibmV4dENvbW1pdFN0YXR1cyI6IlBFTkRJTkciLCJsaXZlRmVlZGJhY2siOnsicmVzb2x2ZWQiOjAsInVucmVzb2x2ZWQiOjAsInRvdGFsIjowLCJsaW5rIjoiZWFzeS1wZWFzeS1naXQtZm9yay1zY2FzY2FyLWZpeC1mYWlsLXRodW5rLW9uLTJkNzZjZS1jdHJscGx1c2IudmVyY2VsLmFwcCJ9fV19
**The latest updates on your projects**. Learn more about [Vercel for Git ↗︎](https://vercel.link/github-learn-more)

| Name | Status | Preview | Comments | Updated (UTC) |
| :--- | :----- | :------ | :------- | :------ |
| **easy-peasy** | 🔄 Building ([Inspect](https://vercel.com/ctrlplusb/easy-peasy/9RLuhf2dxgbNfE82ndvpDtJYqzEu)) | [Visit Preview](https://vercel.live/open-feedback/easy-peasy-git-fork-scascar-fix-fail-thunk-on-2d76ce-ctrlplusb.vercel.app?via=pr-comment-visit-preview-link&passThrough=1) | 💬 [**Add feedback**](https://vercel.live/open-feedback/easy-peasy-git-fork-scascar-fix-fail-thunk-on-2d76ce-ctrlplusb.vercel.app?via=pr-comment-feedback-link) | Aug 11, 2023 10:03am |


jmyrland: hi @scascar !

I think this is a good addition, and an intended effect of the `fail()` helper based on the docs.

> Note: This might introduce unforeseen side-effects, for example users relying on a success thunk after having called an empty helpers.fail().

True, but I can't really see a usecase where this makes sense. If calling `fail()`, I would expect it to trigger a failure. 
However, it's a good thing to mention in the release notes 👍 

Do you see any issues by adding this @ctrlplusb @no-stack-dub-sack ?
no-stack-dub-sack: Hey @jmyrland @scascar - this looks good to me too. No issues from my end! Nice catch :-)

Patch:
diff --git a/index.d.ts b/index.d.ts
index 691c5f9..88c9b4e 100644
--- a/index.d.ts
+++ b/index.d.ts
@@ -731,7 +731,7 @@ export function effectOn<
     actions: Actions<Model>,
     change: Change<Resolvers>,
     helpers: Helpers<Model, StoreModel, Injections>,
-  ) => undefined | void | Dispose,
+  ) => undefined | void | Dispose | Promise<Dispose>,
 ): EffectOn<Model, StoreModel, Injections>;
 
 // #endregion
@@ -1042,8 +1042,10 @@ export interface PersistConfig<Model extends object> {
   mergeStrategy?: 'mergeDeep' | 'mergeShallow' | 'overwrite';
   migrations?: {
     migrationVersion: number;
-    [key: number]: (state: Partial<Model & { [key: string | number]: any }>) => void;
-  }
+    [key: number]: (
+      state: Partial<Model & { [key: string | number]: any }>,
+    ) => void;
+  };
   storage?: 'localStorage' | 'sessionStorage' | PersistStorage;
   transformers?: Array<Transformer>;
 }
diff --git a/src/thunks.js b/src/thunks.js
index f19c4aa..6393ed0 100644
--- a/src/thunks.js
+++ b/src/thunks.js
@@ -77,7 +77,7 @@ export function createThunkActionsCreator(def, _r) {
     let failure = null;
 
     const fail = (_failure) => {
-      failure = _failure;
+      failure = _failure || new Error();
     };
 
     const result = _r.dispatch(() => def.thunkHandler(payload, fail));


Test patch:
diff --git a/tests/listener-actions.test.js b/tests/listener-actions.test.js
index 3966b33..f17b75d 100644
--- a/tests/listener-actions.test.js
+++ b/tests/listener-actions.test.js
@@ -121,6 +121,10 @@ it('listening to a successful or failed thunk, firing an action', async () => {
           helpers.fail('💩');
           return undefined;
         }
+        if (payload === 8) {
+          helpers.fail();
+          return undefined;
+        }
         return 'foo';
       }),
     },
@@ -162,6 +166,20 @@ it('listening to a successful or failed thunk, firing an action', async () => {
   expect(actualTarget.result).toBeUndefined();
   expect(actualTarget.error).toBe('💩');
   expect(store.getState().audit.logs).toEqual(['Added 10', 'Failed to add 7']);
+
+  // ACT
+  await store.getActions().math.add(8);
+
+  // ASSERT
+  expect(actualTarget.type).toBe('@thunk.math.add(fail)');
+  expect(actualTarget.payload).toBe(8);
+  expect(actualTarget.result).toBeUndefined();
+  expect(actualTarget.error).toStrictEqual(new Error());
+  expect(store.getState().audit.logs).toEqual([
+    'Added 10',
+    'Failed to add 7',
+    'Failed to add 8',
+  ]);
 });
 
 it('listening to a failed thunk', async () => {
diff --git a/tests/typescript/effect-on.ts b/tests/typescript/effect-on.ts
index 84ee642..4993de7 100644
--- a/tests/typescript/effect-on.ts
+++ b/tests/typescript/effect-on.ts
@@ -9,6 +9,7 @@ interface TodosModel {
   foo: string;
   setFoo: Action<TodosModel, string>;
   onStateChanged: EffectOn<TodosModel, StoreModel, Injections>;
+  onStateChangedAsync: EffectOn<TodosModel, StoreModel, Injections>;
 }
 
 interface StoreModel {
@@ -52,4 +53,11 @@ const todosModel: TodosModel = {
       return () => console.log('dispose');
     },
   ),
+  onStateChangedAsync: effectOn(
+    [(state) => state.items],
+    // Should not generate error for async effect
+    async (actions, change, helpers) => {
+      return () => console.log('dispose');
+    },
+  ),
 };