From 9cb59ab7d4a1f4ce4b6dea16d57b9cc740a1b49f Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Thu, 8 Sep 2022 21:13:30 +1000 Subject: [PATCH 1/4] Fix issue in instrumenter with `waitFor` --- .../addon-interactions.stories.ts | 23 +++++++++++-------- code/lib/instrumenter/src/instrumenter.ts | 2 +- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/code/examples/angular-cli/src/stories/addons/interactions/addon-interactions.stories.ts b/code/examples/angular-cli/src/stories/addons/interactions/addon-interactions.stories.ts index 3bb0ac900c85..79f27fc7fdd7 100644 --- a/code/examples/angular-cli/src/stories/addons/interactions/addon-interactions.stories.ts +++ b/code/examples/angular-cli/src/stories/addons/interactions/addon-interactions.stories.ts @@ -66,16 +66,19 @@ Submitted.play = async (context) => { const canvas = within(context.canvasElement); await userEvent.click(canvas.getByText('Submit')); - await waitFor(async () => { - await expect( - canvas.getByRole('heading', { - name: /you submitted the following:/i, - }) - ).toBeInTheDocument(); - await expect(canvas.getByTestId('hero-name').textContent).toEqual('Storm'); - await expect(canvas.getByTestId('hero-alterego').textContent).toEqual('Ororo Munroe'); - await expect(canvas.getByTestId('hero-power').textContent).toEqual('Weather Changer'); - }); + await waitFor( + async () => { + await expect( + canvas.getByRole('heading', { + name: /you submitted the following:/i, + }) + ).toBeInTheDocument(); + await expect(canvas.getByTestId('hero-name').textContent).toEqual('Storm'); + await expect(canvas.getByTestId('hero-alterego').textContent).toEqual('Ororo Munroe'); + await expect(canvas.getByTestId('hero-power').textContent).toEqual('Weather Changer'); + }, + { timeout: 2000 } + ); }; export const SubmittedAndEditedAfter: StoryFn = Template.bind({}); diff --git a/code/lib/instrumenter/src/instrumenter.ts b/code/lib/instrumenter/src/instrumenter.ts index 966e7881528e..ff8d5da9649c 100644 --- a/code/lib/instrumenter/src/instrumenter.ts +++ b/code/lib/instrumenter/src/instrumenter.ts @@ -501,7 +501,7 @@ export class Instrumenter { const res = arg(...args); // Reset cursor and ancestors to their original values before we entered the callback. - if (res instanceof Promise) return res.then(restore, restore); + if (res instanceof Promise) return res.finally(restore); restore(); return res; }; From 67dca798b963ebdeb2572931f48e2987198147b0 Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Mon, 12 Sep 2022 12:00:31 +1000 Subject: [PATCH 2/4] Add tests for restoring and fix issue with sync errors not restoring --- .../lib/instrumenter/src/instrumenter.test.ts | 75 +++++++++++++++++++ code/lib/instrumenter/src/instrumenter.ts | 21 ++++-- 2 files changed, 89 insertions(+), 7 deletions(-) diff --git a/code/lib/instrumenter/src/instrumenter.test.ts b/code/lib/instrumenter/src/instrumenter.test.ts index a0a1a0e19d22..95cd22b9d28c 100644 --- a/code/lib/instrumenter/src/instrumenter.test.ts +++ b/code/lib/instrumenter/src/instrumenter.test.ts @@ -1,3 +1,4 @@ +/// ; /* eslint-disable no-underscore-dangle */ import { addons, mockChannel } from '@storybook/addons'; @@ -241,6 +242,43 @@ describe('Instrumenter', () => { ); }); + it('handles exceptions when making calls inside callbacks', () => { + const fn = (callback?: Function) => callback && callback(); + const { fn1, fn2, fn3 } = instrument({ + fn1: fn, + fn2: fn, + fn3: fn, + }); + const error = new Error('foo'); + let thrownError; + fn1(() => { + try { + fn2(() => { + throw error; + }); + } catch (err) { + thrownError = err; + } + fn3(); + }); + expect(callSpy).toHaveBeenCalledWith( + expect.objectContaining({ id: 'kind--story [0] fn1', ancestors: [] }) + ); + expect(callSpy).toHaveBeenCalledWith( + expect.objectContaining({ + id: 'kind--story [0] fn1 [0] fn2', + ancestors: ['kind--story [0] fn1'], + }) + ); + expect(thrownError).toBe(error); + expect(callSpy).toHaveBeenCalledWith( + expect.objectContaining({ + id: 'kind--story [0] fn1 [1] fn3', + ancestors: ['kind--story [0] fn1'], + }) + ); + }); + it('tracks the parent call id for async callbacks', async () => { const fn = (callback?: Function) => Promise.resolve(callback && callback()); const { fn1, fn2, fn3 } = instrument({ fn1: fn, fn2: fn, fn3: fn }); @@ -275,6 +313,43 @@ describe('Instrumenter', () => { ); }); + it('handles exceptions when making calls inside async callbacks', async () => { + const fn = (callback?: Function) => Promise.resolve(callback && callback()); + const { fn1, fn2, fn3 } = instrument({ + fn1: fn, + fn2: fn, + fn3: fn, + }); + const error = new Error('foo'); + let thrownError; + await fn1(async () => { + try { + await fn2(async () => { + throw error; + }); + } catch (err) { + thrownError = err; + } + await fn3(); + }); + expect(callSpy).toHaveBeenCalledWith( + expect.objectContaining({ id: 'kind--story [0] fn1', ancestors: [] }) + ); + expect(callSpy).toHaveBeenCalledWith( + expect.objectContaining({ + id: 'kind--story [0] fn1 [0] fn2', + ancestors: ['kind--story [0] fn1'], + }) + ); + expect(thrownError).toBe(error); + expect(callSpy).toHaveBeenCalledWith( + expect.objectContaining({ + id: 'kind--story [0] fn1 [1] fn3', + ancestors: ['kind--story [0] fn1'], + }) + ); + }); + it('emits a "sync" event with debounce after a patched function is invoked', () => { const { fn } = instrument({ fn: (...args: any) => {} }, { intercept: true }); jest.useFakeTimers(); diff --git a/code/lib/instrumenter/src/instrumenter.ts b/code/lib/instrumenter/src/instrumenter.ts index ff8d5da9649c..3ad1833415d6 100644 --- a/code/lib/instrumenter/src/instrumenter.ts +++ b/code/lib/instrumenter/src/instrumenter.ts @@ -497,13 +497,20 @@ export class Instrumenter { this.setState(call.storyId, { cursor: 0, ancestors: [...ancestors, call.id] }); const restore = () => this.setState(call.storyId, { cursor, ancestors }); - // Invoke the actual callback function. - const res = arg(...args); - - // Reset cursor and ancestors to their original values before we entered the callback. - if (res instanceof Promise) return res.finally(restore); - restore(); - return res; + let restored = false; + try { + // Invoke the actual callback function. + const res = arg(...args); + + // Reset cursor and ancestors to their original values before we entered the callback. + if (res instanceof Promise) { + restored = true; + return res.finally(restore); + } + return res; + } finally { + if (!restored) restore(); + } }; }); From 673452595090f8a6b9a9d0a29cf2d02175f89c40 Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Mon, 12 Sep 2022 17:26:14 +1000 Subject: [PATCH 3/4] Follow up on code comments --- code/lib/instrumenter/src/instrumenter.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/code/lib/instrumenter/src/instrumenter.ts b/code/lib/instrumenter/src/instrumenter.ts index 3ad1833415d6..9d22175073a3 100644 --- a/code/lib/instrumenter/src/instrumenter.ts +++ b/code/lib/instrumenter/src/instrumenter.ts @@ -497,19 +497,18 @@ export class Instrumenter { this.setState(call.storyId, { cursor: 0, ancestors: [...ancestors, call.id] }); const restore = () => this.setState(call.storyId, { cursor, ancestors }); - let restored = false; + // Invoke the actual callback function, taking care to reset the cursor and ancestors + // to their original values before we entered the callback, once the callback completes. + let willRestore = false; try { - // Invoke the actual callback function. const res = arg(...args); - - // Reset cursor and ancestors to their original values before we entered the callback. if (res instanceof Promise) { - restored = true; + willRestore = true; return res.finally(restore); } return res; } finally { - if (!restored) restore(); + if (!willRestore) restore(); } }; }); From 274101c21ce34533eedf24b4995611e69ad1304d Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Mon, 12 Sep 2022 17:26:43 +1000 Subject: [PATCH 4/4] Add useful comment --- code/lib/instrumenter/src/instrumenter.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/code/lib/instrumenter/src/instrumenter.ts b/code/lib/instrumenter/src/instrumenter.ts index 9d22175073a3..b8c9cce1c47a 100644 --- a/code/lib/instrumenter/src/instrumenter.ts +++ b/code/lib/instrumenter/src/instrumenter.ts @@ -503,7 +503,7 @@ export class Instrumenter { try { const res = arg(...args); if (res instanceof Promise) { - willRestore = true; + willRestore = true; // We need to wait for the promise to finish before restoring return res.finally(restore); } return res;