From 9068e31098bc0f115f86bc0ee9f2fec47f83ac2a Mon Sep 17 00:00:00 2001 From: Logan Allred Date: Mon, 1 May 2023 13:36:14 -0600 Subject: [PATCH 1/6] React decorators can conditionally render children Fixes #15223 Custom preview-api hooks assumed that all decorators were always rendered however if a custom decorator conditionally rendered it's children, then not all decorators would get rendered, especially jsxDecorator. This would result in the error "Rendered more hooks than during the previous render." This removes the assumption that all decorators render every time and relies on each decorator to register itself during MOUNT phase which is handled when each decorator goes through `hookify` --- .../preview-api/src/modules/addons/hooks.ts | 4 ++-- .../src/modules/store/hooks.test.ts | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/code/lib/preview-api/src/modules/addons/hooks.ts b/code/lib/preview-api/src/modules/addons/hooks.ts index e695938a444d..ce1643315824 100644 --- a/code/lib/preview-api/src/modules/addons/hooks.ts +++ b/code/lib/preview-api/src/modules/addons/hooks.ts @@ -70,7 +70,7 @@ export class HooksContext init() { this.hookListsMap = new WeakMap(); this.mountedDecorators = new Set(); - this.prevMountedDecorators = this.mountedDecorators; + this.prevMountedDecorators = new Set(); this.currentHooks = []; this.nextHookIndex = 0; this.currentPhase = 'NONE'; @@ -191,7 +191,7 @@ export const applyHooks = ); return (context) => { const { hooks } = context as { hooks: HooksContext }; - hooks.prevMountedDecorators = hooks.mountedDecorators; + hooks.prevMountedDecorators ??= new Set(); hooks.mountedDecorators = new Set([storyFn, ...decorators]); hooks.currentContext = context; hooks.hasUpdates = false; diff --git a/code/lib/preview-api/src/modules/store/hooks.test.ts b/code/lib/preview-api/src/modules/store/hooks.test.ts index bbd72f95b0a5..655a41249b4f 100644 --- a/code/lib/preview-api/src/modules/store/hooks.test.ts +++ b/code/lib/preview-api/src/modules/store/hooks.test.ts @@ -147,6 +147,25 @@ describe('Preview hooks', () => { run(story, [decorator]); expect(effect).toHaveBeenCalledTimes(1); }); + it('handles decorator conditionally rendering the story', () => { + const effect = jest.fn(); + const story = () => { + useEffect(effect, []); + }; + const decorator = (storyFn: any) => { + const [counter, setCounter] = useState(0); + useEffect(() => { + setCounter((prevCounter) => prevCounter + 1); + }, [counter]); + if (counter % 2 === 1) storyFn(); + return 'placeholder while waiting'; + }; + run(story, [decorator]); + run(story, [decorator]); + run(story, [decorator]); + run(story, [decorator]); + expect(effect).toHaveBeenCalledTimes(2); + }); it('retriggers the effect if some of the deps are changed', () => { const effect = jest.fn(); let counter = 0; From 6822e4d16f32a2875afaa14f0ef3ec14da6a3942 Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Mon, 29 May 2023 20:49:21 +1000 Subject: [PATCH 2/6] Use originalStoryFunction as that exists in all renderers --- .../template/stories/decorators.stories.ts | 64 +++++++++++++++++++ 1 file changed, 64 insertions(+) create mode 100644 code/lib/store/template/stories/decorators.stories.ts diff --git a/code/lib/store/template/stories/decorators.stories.ts b/code/lib/store/template/stories/decorators.stories.ts new file mode 100644 index 000000000000..175164f6cdc7 --- /dev/null +++ b/code/lib/store/template/stories/decorators.stories.ts @@ -0,0 +1,64 @@ +import { global as globalThis } from '@storybook/global'; +import type { + ArgsStoryFn, + PartialStoryFn, + PlayFunctionContext, + StoryContext, +} from '@storybook/types'; +import { within } from '@storybook/testing-library'; +import { expect } from '@storybook/jest'; +import { useEffect } from '@storybook/preview-api'; +import { STORY_ARGS_UPDATED, UPDATE_STORY_ARGS, RESET_STORY_ARGS } from '@storybook/core-events'; + +export default { + component: globalThis.Components.Pre, + parameters: { useProjectDecorator: true }, + decorators: [ + (storyFn: PartialStoryFn, context: StoryContext) => + storyFn({ args: { ...context.args, text: `component ${context.args['text']}` } }), + ], +}; + +export const Inheritance = { + decorators: [ + (storyFn: PartialStoryFn, context: StoryContext) => + storyFn({ args: { ...context.args, text: `story ${context.args['text']}` } }), + ], + args: { + text: 'starting', + }, + play: async ({ canvasElement }: PlayFunctionContext) => { + const canvas = within(canvasElement); + await expect(canvas.getByTestId('pre').innerText).toEqual('story component project starting'); + }, +}; + +export const Hooks = { + decorators: [ + // decorator that uses hooks + (storyFn: PartialStoryFn, context: StoryContext) => { + useEffect(() => {}); + return storyFn({ args: { ...context.args, text: `story ${context.args['text']}` } }); + }, + // conditional decorator, runs before the above + (storyFn: PartialStoryFn, context: StoryContext) => + context.args.condition + ? storyFn() + : (context.originalStoryFn as ArgsStoryFn)(context.args, context), + ], + args: { + text: 'text', + condition: true, + }, + play: async ({ id, args }: PlayFunctionContext) => { + const channel = globalThis.__STORYBOOK_ADDONS_CHANNEL__; + await channel.emit(RESET_STORY_ARGS, { storyId: id }); + await new Promise((resolve) => channel.once(STORY_ARGS_UPDATED, resolve)); + + await channel.emit(UPDATE_STORY_ARGS, { + storyId: id, + updatedArgs: { condition: !args.condition }, + }); + await new Promise((resolve) => channel.once(STORY_ARGS_UPDATED, resolve)); + }, +}; From cfeae7181eca40d04aef0f9a6f4ec31821232e43 Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Tue, 30 May 2023 19:15:36 +1000 Subject: [PATCH 3/6] Try and make decorator story more consistent --- code/lib/store/template/stories/decorators.stories.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/code/lib/store/template/stories/decorators.stories.ts b/code/lib/store/template/stories/decorators.stories.ts index 175164f6cdc7..7104627ae2a2 100644 --- a/code/lib/store/template/stories/decorators.stories.ts +++ b/code/lib/store/template/stories/decorators.stories.ts @@ -57,7 +57,13 @@ export const Hooks = { await channel.emit(UPDATE_STORY_ARGS, { storyId: id, - updatedArgs: { condition: !args.condition }, + updatedArgs: { condition: false }, + }); + await new Promise((resolve) => channel.once(STORY_ARGS_UPDATED, resolve)); + + await channel.emit(UPDATE_STORY_ARGS, { + storyId: id, + updatedArgs: { condition: true }, }); await new Promise((resolve) => channel.once(STORY_ARGS_UPDATED, resolve)); }, From 213117bac4292ed58f017022eaf76891cfb41a8b Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Thu, 1 Jun 2023 12:34:16 +1000 Subject: [PATCH 4/6] Tweak story again --- code/lib/store/template/stories/decorators.stories.ts | 6 ------ 1 file changed, 6 deletions(-) diff --git a/code/lib/store/template/stories/decorators.stories.ts b/code/lib/store/template/stories/decorators.stories.ts index 7104627ae2a2..f3ab678116fd 100644 --- a/code/lib/store/template/stories/decorators.stories.ts +++ b/code/lib/store/template/stories/decorators.stories.ts @@ -60,11 +60,5 @@ export const Hooks = { updatedArgs: { condition: false }, }); await new Promise((resolve) => channel.once(STORY_ARGS_UPDATED, resolve)); - - await channel.emit(UPDATE_STORY_ARGS, { - storyId: id, - updatedArgs: { condition: true }, - }); - await new Promise((resolve) => channel.once(STORY_ARGS_UPDATED, resolve)); }, }; From 01ab4508ce72c72b52a5a28a225400bee3f29846 Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Fri, 2 Jun 2023 10:58:46 +1000 Subject: [PATCH 5/6] Move the new decorators stories to the right place --- .../template/stories/decorators.stories.ts | 39 ++++++++++- .../template/stories/decorators.stories.ts | 64 ------------------- 2 files changed, 38 insertions(+), 65 deletions(-) delete mode 100644 code/lib/store/template/stories/decorators.stories.ts diff --git a/code/lib/preview-api/template/stories/decorators.stories.ts b/code/lib/preview-api/template/stories/decorators.stories.ts index 1e07554ad9e3..f3ab678116fd 100644 --- a/code/lib/preview-api/template/stories/decorators.stories.ts +++ b/code/lib/preview-api/template/stories/decorators.stories.ts @@ -1,7 +1,14 @@ import { global as globalThis } from '@storybook/global'; -import type { PartialStoryFn, PlayFunctionContext, StoryContext } from '@storybook/types'; +import type { + ArgsStoryFn, + PartialStoryFn, + PlayFunctionContext, + StoryContext, +} from '@storybook/types'; import { within } from '@storybook/testing-library'; import { expect } from '@storybook/jest'; +import { useEffect } from '@storybook/preview-api'; +import { STORY_ARGS_UPDATED, UPDATE_STORY_ARGS, RESET_STORY_ARGS } from '@storybook/core-events'; export default { component: globalThis.Components.Pre, @@ -25,3 +32,33 @@ export const Inheritance = { await expect(canvas.getByTestId('pre').innerText).toEqual('story component project starting'); }, }; + +export const Hooks = { + decorators: [ + // decorator that uses hooks + (storyFn: PartialStoryFn, context: StoryContext) => { + useEffect(() => {}); + return storyFn({ args: { ...context.args, text: `story ${context.args['text']}` } }); + }, + // conditional decorator, runs before the above + (storyFn: PartialStoryFn, context: StoryContext) => + context.args.condition + ? storyFn() + : (context.originalStoryFn as ArgsStoryFn)(context.args, context), + ], + args: { + text: 'text', + condition: true, + }, + play: async ({ id, args }: PlayFunctionContext) => { + const channel = globalThis.__STORYBOOK_ADDONS_CHANNEL__; + await channel.emit(RESET_STORY_ARGS, { storyId: id }); + await new Promise((resolve) => channel.once(STORY_ARGS_UPDATED, resolve)); + + await channel.emit(UPDATE_STORY_ARGS, { + storyId: id, + updatedArgs: { condition: false }, + }); + await new Promise((resolve) => channel.once(STORY_ARGS_UPDATED, resolve)); + }, +}; diff --git a/code/lib/store/template/stories/decorators.stories.ts b/code/lib/store/template/stories/decorators.stories.ts deleted file mode 100644 index f3ab678116fd..000000000000 --- a/code/lib/store/template/stories/decorators.stories.ts +++ /dev/null @@ -1,64 +0,0 @@ -import { global as globalThis } from '@storybook/global'; -import type { - ArgsStoryFn, - PartialStoryFn, - PlayFunctionContext, - StoryContext, -} from '@storybook/types'; -import { within } from '@storybook/testing-library'; -import { expect } from '@storybook/jest'; -import { useEffect } from '@storybook/preview-api'; -import { STORY_ARGS_UPDATED, UPDATE_STORY_ARGS, RESET_STORY_ARGS } from '@storybook/core-events'; - -export default { - component: globalThis.Components.Pre, - parameters: { useProjectDecorator: true }, - decorators: [ - (storyFn: PartialStoryFn, context: StoryContext) => - storyFn({ args: { ...context.args, text: `component ${context.args['text']}` } }), - ], -}; - -export const Inheritance = { - decorators: [ - (storyFn: PartialStoryFn, context: StoryContext) => - storyFn({ args: { ...context.args, text: `story ${context.args['text']}` } }), - ], - args: { - text: 'starting', - }, - play: async ({ canvasElement }: PlayFunctionContext) => { - const canvas = within(canvasElement); - await expect(canvas.getByTestId('pre').innerText).toEqual('story component project starting'); - }, -}; - -export const Hooks = { - decorators: [ - // decorator that uses hooks - (storyFn: PartialStoryFn, context: StoryContext) => { - useEffect(() => {}); - return storyFn({ args: { ...context.args, text: `story ${context.args['text']}` } }); - }, - // conditional decorator, runs before the above - (storyFn: PartialStoryFn, context: StoryContext) => - context.args.condition - ? storyFn() - : (context.originalStoryFn as ArgsStoryFn)(context.args, context), - ], - args: { - text: 'text', - condition: true, - }, - play: async ({ id, args }: PlayFunctionContext) => { - const channel = globalThis.__STORYBOOK_ADDONS_CHANNEL__; - await channel.emit(RESET_STORY_ARGS, { storyId: id }); - await new Promise((resolve) => channel.once(STORY_ARGS_UPDATED, resolve)); - - await channel.emit(UPDATE_STORY_ARGS, { - storyId: id, - updatedArgs: { condition: false }, - }); - await new Promise((resolve) => channel.once(STORY_ARGS_UPDATED, resolve)); - }, -}; From 85f723b7effa333d676482a55c516afd8dc7937a Mon Sep 17 00:00:00 2001 From: Michael Shilman Date: Wed, 7 Jun 2023 13:36:01 +0800 Subject: [PATCH 6/6] Add Vue2/3 issue to code comment --- code/lib/preview-api/template/stories/decorators.stories.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/code/lib/preview-api/template/stories/decorators.stories.ts b/code/lib/preview-api/template/stories/decorators.stories.ts index f3ab678116fd..a36eb100a739 100644 --- a/code/lib/preview-api/template/stories/decorators.stories.ts +++ b/code/lib/preview-api/template/stories/decorators.stories.ts @@ -33,6 +33,8 @@ export const Inheritance = { }, }; +// NOTE this story is currently broken in Chromatic for both Vue2/Vue3 +// Issue: https://github.com/storybookjs/storybook/issues/22945 export const Hooks = { decorators: [ // decorator that uses hooks