From 6085f19265a619f4827a14a2cae2c61216e2ee4c Mon Sep 17 00:00:00 2001
From: Repraance <reprance@gmail.com>
Date: Mon, 10 Oct 2022 09:41:40 +0800
Subject: [PATCH] fix(hook): seriesHook should discard handlers that returns
 undefined (#441)

---
 .../hooks/asyncSeriesWaterfallHook.test.ts    |  38 +++-
 .../src/__tests__/hooks/syncBailHook.test.ts  |  78 ++++----
 .../hook/src/__tests__/hooks/syncHook.test.ts | 183 ++++++++++--------
 .../__tests__/hooks/syncWaterfallHook.test.ts | 111 ++++++++---
 packages/hook/src/hooks.ts                    |  10 +-
 5 files changed, 270 insertions(+), 150 deletions(-)

diff --git a/packages/hook/src/__tests__/hooks/asyncSeriesWaterfallHook.test.ts b/packages/hook/src/__tests__/hooks/asyncSeriesWaterfallHook.test.ts
index 94763ea6d..a39c9550a 100644
--- a/packages/hook/src/__tests__/hooks/asyncSeriesWaterfallHook.test.ts
+++ b/packages/hook/src/__tests__/hooks/asyncSeriesWaterfallHook.test.ts
@@ -34,6 +34,40 @@ describe('AsyncSeriesWaterfallHook', () => {
   });
 
   test('the initial value of the hook handler should be the return value of the previous hook handler', async () => {
+    const hook = createAsyncSeriesWaterfallHook<number, number>();
+    const handler1: AsyncSeriesWaterfallHookHandler<number, number> = jest.fn(
+      (i, e) => {
+        expect(i).toBe(1);
+        expect(e).toBe(2);
+        return 5;
+      }
+    );
+
+    const handler2: AsyncSeriesWaterfallHookHandler<number, number> = jest.fn(
+      async (i, e) => {
+        expect(i).toBe(5);
+        expect(e).toBe(2);
+        return 10;
+      }
+    );
+
+    const handler3: AsyncSeriesWaterfallHookHandler<number, number> = jest.fn(
+      (i, e) => {
+        expect(i).toBe(10);
+        expect(e).toBe(2);
+        return 8;
+      }
+    );
+
+    hook.use(handler1, handler2, handler3);
+    const result = await hook.run(1, 2);
+    expect(result).toBe(8);
+    expect(handler1).toBeCalledTimes(1);
+    expect(handler2).toBeCalledTimes(1);
+    expect(handler3).toBeCalledTimes(1);
+  });
+
+  test('the handler that returns undefined should execute but the result should be discarded', async () => {
     const hook = createAsyncSeriesWaterfallHook<number | undefined, number>();
     const handler1: AsyncSeriesWaterfallHookHandler<
       number | undefined,
@@ -48,7 +82,7 @@ describe('AsyncSeriesWaterfallHook', () => {
       number | undefined,
       number
     > = jest.fn(async (i, e) => {
-      expect(i).toBe(undefined);
+      expect(i).toBe(1);
       expect(e).toBe(2);
       return 10;
     });
@@ -64,7 +98,7 @@ describe('AsyncSeriesWaterfallHook', () => {
 
     hook.use(handler1, handler2, handler3);
     const result = await hook.run(1, 2);
-    expect(result).toBeUndefined();
+    expect(result).toBe(10);
     expect(handler1).toBeCalledTimes(1);
     expect(handler2).toBeCalledTimes(1);
     expect(handler3).toBeCalledTimes(1);
diff --git a/packages/hook/src/__tests__/hooks/syncBailHook.test.ts b/packages/hook/src/__tests__/hooks/syncBailHook.test.ts
index 00eb760dd..2b139958c 100644
--- a/packages/hook/src/__tests__/hooks/syncBailHook.test.ts
+++ b/packages/hook/src/__tests__/hooks/syncBailHook.test.ts
@@ -1,56 +1,56 @@
-import { createSyncBailHook } from '../../index';
+import { createSyncBailHook, SyncBailHookHandler } from '../../index';
 
-beforeEach(() => {});
-
-afterEach(async () => {});
-
-describe('syncBailHook', () => {
-  console.log = jest.fn();
-  test('order', async () => {
+describe('SyncBailHook', () => {
+  test('hooks should run in sequence', () => {
     const hook = createSyncBailHook<void>();
+    const logs: string[] = [];
     hook.use(() => {
-      console.log('hook 1');
+      logs.push('hook 1');
     });
     hook.use(() => {
-      console.log('hook 2');
+      logs.push('hook 2');
     });
     hook.run();
-    expect(console.log).toHaveBeenNthCalledWith(1, 'hook 1');
-    expect(console.log).toHaveBeenNthCalledWith(2, 'hook 2');
+    expect(logs).toEqual(['hook 1', 'hook 2']);
   });
 
-  test('with initialValue', async () => {
-    const hook = createSyncBailHook<number>();
-    hook.use(e => {
-      console.log(e);
-    });
-    hook.run(10);
-    expect(console.log).toHaveBeenCalledWith(10);
-  });
-  test('with extraArg', async () => {
-    const hook = createSyncBailHook<void, number>();
-    hook.use(e => {
-      console.log(e);
-    });
-    hook.run(10);
-    expect(console.log).toHaveBeenCalledWith(10);
+  test('the params of run() should be as the params of the hook handler', () => {
+    const hook = createSyncBailHook<number, number>();
+    const handler: SyncBailHookHandler<number, number, void> = jest.fn(
+      (i, e) => {
+        expect(i).toBe(1);
+        expect(e).toBe(2);
+      }
+    );
+    hook.use(handler);
+    hook.run(1, 2);
+    expect(handler).toBeCalledTimes(1);
   });
 
-  test('with initialValue and extraArg', async () => {
-    const hook = createSyncBailHook<number, number>();
-    hook.use((i, e) => {
-      console.log(i, e);
-    });
-    hook.run(10, 20);
-    expect(console.log).toHaveBeenCalledWith(10, 20);
+  test('hook result should be the return value of the first handler that returns value', () => {
+    const hook = createSyncBailHook<void, void, number>();
+    const handler1 = jest.fn(() => {});
+    const handler2 = jest.fn(() => 10);
+    const handler3 = jest.fn(() => 20);
+    hook.use(handler1);
+    hook.use(handler2);
+    hook.use(handler3);
+    const result = hook.run();
+    expect(result).toBe(10);
+    expect(handler1).toBeCalledTimes(1);
+    expect(handler2).toBeCalledTimes(1);
+    expect(handler3).toBeCalledTimes(0);
   });
 
-  test('with return value', async () => {
+  test('hook result should be undefined if none of the handlers returns value', () => {
     const hook = createSyncBailHook<void, void, number>();
-    hook.use(() => {});
-    hook.use(() => 10);
-    hook.use(() => 20);
+    const handler1 = jest.fn(() => {});
+    const handler2 = jest.fn(() => {});
+    hook.use(handler1);
+    hook.use(handler2);
     const result = hook.run();
-    expect(result).toStrictEqual(10);
+    expect(result).toBe(undefined);
+    expect(handler1).toBeCalledTimes(1);
+    expect(handler2).toBeCalledTimes(1);
   });
 });
diff --git a/packages/hook/src/__tests__/hooks/syncHook.test.ts b/packages/hook/src/__tests__/hooks/syncHook.test.ts
index 2413d6ac5..77cbd5280 100644
--- a/packages/hook/src/__tests__/hooks/syncHook.test.ts
+++ b/packages/hook/src/__tests__/hooks/syncHook.test.ts
@@ -1,107 +1,130 @@
-import { createSyncHook } from '../../hooks';
+import { SyncHookHandler, createSyncHook } from '../../hooks';
 
-beforeEach(() => {});
-
-afterEach(async () => {});
-
-describe('syncHook', () => {
-  console.log = jest.fn();
-  test('order', async () => {
+describe('AsyncSeriesHook', () => {
+  test('hooks should run in sequence', () => {
     const hook = createSyncHook<void>();
+    const logs: string[] = [];
     hook.use(() => {
-      console.log('hook 1');
+      logs.push('hook 1');
     });
     hook.use(() => {
-      console.log('hook 2');
+      logs.push('hook 2');
     });
     hook.run();
-    expect(console.log).toHaveBeenNthCalledWith(1, 'hook 1');
-    expect(console.log).toHaveBeenNthCalledWith(2, 'hook 2');
+    expect(logs).toEqual(['hook 1', 'hook 2']);
   });
 
-  test('initialValue-normal extraArg-normal', async () => {
+  test('the params of run() should be as the params of the hook handler', () => {
     const hook = createSyncHook<number, number>();
-    hook.use((i, e) => {
-      console.log(i, e);
-    });
-    hook.run(10, 20);
-    expect(console.log).toHaveBeenCalledWith(10, 20);
-  });
-  test('initialValue-void extraArg-normal', async () => {
-    const hook = createSyncHook<void, number>();
-    hook.use(e => {
-      console.log(e);
+    const handler: SyncHookHandler<number, number, void> = jest.fn((i, e) => {
+      expect(i).toBe(1);
+      expect(e).toBe(2);
     });
-    hook.run(10);
-    expect(console.log).toHaveBeenCalledWith(10);
+    hook.use(handler);
+    hook.run(1, 2);
+    expect(handler).toBeCalledTimes(1);
   });
 
-  test('initialValue-any extraArg-normal', async () => {
-    const hook = createSyncHook<any, number>();
-    hook.use((i, e) => {
-      console.log(i, e);
-    });
-    hook.run('10', 20);
-    expect(console.log).toHaveBeenCalledWith('10', 20);
+  test('result should be array', () => {
+    const hook = createSyncHook<void, void, number | void>();
+    hook.use(() => 10);
+    hook.use(() => {});
+    hook.use(() => 20);
+    const result = hook.run();
+    expect(result).toStrictEqual([10, undefined, 20]);
   });
 
-  test('initialValue-normal extraArg-void', async () => {
-    const hook = createSyncHook<number, void>();
-    hook.use(i => {
-      console.log(i);
+  describe('types', () => {
+    test('initialValue-normal extraArg-normal', async () => {
+      let initialValue: number;
+      let extraArg: number;
+      const hook = createSyncHook<number, number>();
+      hook.use((i, e) => {
+        initialValue = i;
+        extraArg = e;
+      });
+      hook.run(10, 20);
+      expect(true).toBe(true);
     });
-    hook.run(10);
-    expect(console.log).toHaveBeenCalledWith(10);
-  });
-  test('initialValue-void extraArg-void', async () => {
-    const hook = createSyncHook<void, void>();
-    hook.use(() => {
-      console.log('test');
+    test('initialValue-void extraArg-normal', async () => {
+      let extraArg: number;
+      const hook = createSyncHook<void, number>();
+      hook.use(e => {
+        extraArg = e;
+      });
+      hook.run(10);
+      expect(true).toBe(true);
     });
-    hook.run();
-    expect(console.log).toHaveBeenCalledWith('test');
-  });
 
-  test('initialValue-any extraArg-void', async () => {
-    const hook = createSyncHook<any, void>();
-    hook.use(i => {
-      console.log(i);
+    test('initialValue-any extraArg-normal', async () => {
+      let initialValue: any;
+      let extraArg: number;
+      const hook = createSyncHook<any, number>();
+      hook.use((i, e) => {
+        initialValue = i;
+        extraArg = e;
+      });
+      hook.run('10', 20);
+      expect(true).toBe(true);
     });
-    hook.run('test any');
-    expect(console.log).toHaveBeenCalledWith('test any');
-  });
 
-  test('initialValue-normal extraArg-any', async () => {
-    const hook = createSyncHook<number, any>();
-    hook.use((i, e) => {
-      console.log(i, e);
+    test('initialValue-normal extraArg-void', async () => {
+      let initialValue: number;
+      const hook = createSyncHook<number, void>();
+      hook.use(i => {
+        initialValue = i;
+      });
+      hook.run(10);
+      expect(true).toBe(true);
     });
-    hook.run(10, '20');
-    expect(console.log).toHaveBeenCalledWith(10, '20');
-  });
-  test('initialValue-void extraArg-any', async () => {
-    const hook = createSyncHook<void, any>();
-    hook.use(() => {
-      console.log('test any');
+    test('initialValue-void extraArg-void', async () => {
+      const hook = createSyncHook<void, void>();
+      hook.use(() => {});
+      hook.run();
+      expect(true).toBe(true);
     });
-    hook.run('test any');
-    expect(console.log).toHaveBeenCalledWith('test');
-  });
 
-  test('initialValue-any extraArg-any', async () => {
-    const hook = createSyncHook<any, any>();
-    hook.use((i, e) => {
-      console.log(i, e);
+    test('initialValue-any extraArg-void', async () => {
+      let initialValue: any;
+      const hook = createSyncHook<any, void>();
+      hook.use(i => {
+        initialValue = i;
+      });
+      hook.run('test any');
+      expect(true).toBe(true);
     });
-    hook.run('test 1', 'test 2');
-    expect(console.log).toHaveBeenCalledWith('test 1', 'test 2');
-  });
 
-  test('with return value', async () => {
-    const hook = createSyncHook<void, void, number>();
-    hook.use(() => 10);
-    hook.use(() => 20);
-    const result = hook.run();
-    expect(result).toStrictEqual([10, 20]);
+    test('initialValue-normal extraArg-any', async () => {
+      let initialValue: number;
+      let extraArg: any;
+      const hook = createSyncHook<number, any>();
+      hook.use((i, e) => {
+        initialValue = i;
+        extraArg = e;
+      });
+      hook.run(10, '20');
+      expect(true).toBe(true);
+    });
+    test('initialValue-void extraArg-any', async () => {
+      let extraArg: any;
+      const hook = createSyncHook<void, any>();
+      hook.use(e => {
+        extraArg = e;
+      });
+      hook.run('test any');
+      expect(true).toBe(true);
+    });
+
+    test('initialValue-any extraArg-any', async () => {
+      let initialValue: any;
+      let extraArg: any;
+      const hook = createSyncHook<any, any>();
+      hook.use((i, e) => {
+        initialValue = i;
+        extraArg = e;
+      });
+      hook.run('test 1', 'test 2');
+      expect(true).toBe(true);
+    });
   });
 });
diff --git a/packages/hook/src/__tests__/hooks/syncWaterfallHook.test.ts b/packages/hook/src/__tests__/hooks/syncWaterfallHook.test.ts
index 34296c997..2cb48154f 100644
--- a/packages/hook/src/__tests__/hooks/syncWaterfallHook.test.ts
+++ b/packages/hook/src/__tests__/hooks/syncWaterfallHook.test.ts
@@ -1,38 +1,95 @@
-import { createSyncWaterfallHook } from '../../hooks';
+import { SyncWaterfallHookHandler, createSyncWaterfallHook } from '../../hooks';
 
-beforeEach(() => {});
-
-afterEach(async () => {});
-
-describe('syncHook', () => {
-  console.log = jest.fn();
-  test('not used', () => {
-    const hook = createSyncWaterfallHook<number>();
-    const result = hook.run(10);
-    expect(result).toBe(10);
-  });
-  test('initialValue-void', async () => {
+describe('SyncWaterfallHook', () => {
+  test('hooks should run in sequence', () => {
     const hook = createSyncWaterfallHook<void>();
+    const logs: string[] = [];
     hook.use(() => {
-      console.log('hook 1');
+      logs.push('hook 1');
     });
     hook.use(() => {
-      console.log('hook 2');
+      logs.push('hook 2');
     });
     hook.run();
-    expect(console.log).toHaveBeenNthCalledWith(1, 'hook 1');
-    expect(console.log).toHaveBeenNthCalledWith(2, 'hook 2');
+    expect(logs).toEqual(['hook 1', 'hook 2']);
   });
 
-  test('initialValue-normal extraArg-void', async () => {
-    const hook = createSyncWaterfallHook<number, void>();
-    hook.use(i => {
-      return i * 2;
-    });
-    hook.use(i => {
-      return i * 3;
-    });
-    const result = hook.run(10);
-    expect(result).toBe(60);
+  test('the params of run() should be as the params of the hook handler', async () => {
+    const hook = createSyncWaterfallHook<number, number>();
+    const handler: SyncWaterfallHookHandler<number, number> = jest.fn(
+      (i, e) => {
+        expect(i).toBe(1);
+        expect(e).toBe(2);
+        return i + e;
+      }
+    );
+    hook.use(handler);
+    await hook.run(1, 2);
+    expect(handler).toBeCalledTimes(1);
+  });
+
+  test('the initial value of the hook handler should be the return value of the previous hook handler', () => {
+    const hook = createSyncWaterfallHook<number, number>();
+    const handler1: SyncWaterfallHookHandler<number, number> = jest.fn(
+      (i, e) => {
+        expect(i).toBe(1);
+        expect(e).toBe(2);
+        return 5;
+      }
+    );
+
+    const handler2: SyncWaterfallHookHandler<number, number> = jest.fn(
+      (i, e) => {
+        expect(i).toBe(5);
+        expect(e).toBe(2);
+        return 10;
+      }
+    );
+
+    const handler3: SyncWaterfallHookHandler<number, number> = jest.fn(
+      (i, e) => {
+        expect(i).toBe(10);
+        expect(e).toBe(2);
+        return 8;
+      }
+    );
+
+    hook.use(handler1, handler2, handler3);
+    const result = hook.run(1, 2);
+    expect(result).toBe(8);
+    expect(handler1).toBeCalledTimes(1);
+    expect(handler2).toBeCalledTimes(1);
+    expect(handler3).toBeCalledTimes(1);
+  });
+
+  test('handlers that return undefined should execute but the result should be discarded', () => {
+    const hook = createSyncWaterfallHook<number | undefined, number>();
+    const handler1: SyncWaterfallHookHandler<number | undefined, number> =
+      jest.fn((i, e) => {
+        expect(i).toBe(1);
+        expect(e).toBe(2);
+        return undefined;
+      });
+
+    const handler2: SyncWaterfallHookHandler<number | undefined, number> =
+      jest.fn((i, e) => {
+        expect(i).toBe(1);
+        expect(e).toBe(2);
+        return 10;
+      });
+
+    const handler3: SyncWaterfallHookHandler<number | undefined, number> =
+      jest.fn((i, e) => {
+        expect(i).toBe(10);
+        expect(e).toBe(2);
+        return undefined;
+      });
+
+    hook.use(handler1, handler2, handler3);
+    const result = hook.run(1, 2);
+    expect(result).toBe(10);
+    expect(handler1).toBeCalledTimes(1);
+    expect(handler2).toBeCalledTimes(1);
+    expect(handler3).toBeCalledTimes(1);
   });
 });
diff --git a/packages/hook/src/hooks.ts b/packages/hook/src/hooks.ts
index 44e404346..f6a775e9d 100644
--- a/packages/hook/src/hooks.ts
+++ b/packages/hook/src/hooks.ts
@@ -253,7 +253,10 @@ export const createSyncWaterfallHook = <
     for (let i = 0; i < _handlers.length; i++) {
       const handler = _handlers[i];
       // @ts-ignore
-      currentParam = handler(currentParam, ...otherArgs);
+      const result = handler(currentParam, ...otherArgs);
+      if (result !== undefined) {
+        currentParam = result;
+      }
     }
     return currentParam;
   };
@@ -377,7 +380,10 @@ export const createAsyncSeriesWaterfallHook = <
     for (let i = 0; i < _handlers.length; i++) {
       const handler = _handlers[i];
       // @ts-ignore
-      currentParam = await handler(currentParam, ...otherArgs);
+      const result = await handler(currentParam, ...otherArgs);
+      if (result !== undefined) {
+        currentParam = result;
+      }
     }
     return currentParam;
   };