From ec3c604ba543b1ded23f52e24c880693c596bc94 Mon Sep 17 00:00:00 2001 From: Kasper Peulen Date: Wed, 3 Jul 2024 11:40:19 +0200 Subject: [PATCH 1/5] Refactor phases to always run in order loading -> rendering -> playing --- .../preview-web/render/StoryRender.test.ts | 60 ++++++++--------- .../modules/preview-web/render/StoryRender.ts | 64 ++++++++++++------- code/core/src/preview-errors.ts | 23 +++++++ 3 files changed, 91 insertions(+), 56 deletions(-) diff --git a/code/core/src/preview-api/modules/preview-web/render/StoryRender.test.ts b/code/core/src/preview-api/modules/preview-web/render/StoryRender.test.ts index dee18cba4c1e..dce11cac9ad8 100644 --- a/code/core/src/preview-api/modules/preview-web/render/StoryRender.test.ts +++ b/code/core/src/preview-api/modules/preview-web/render/StoryRender.test.ts @@ -1,5 +1,5 @@ // @vitest-environment happy-dom -import { describe, it, expect, vi } from 'vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; import { Channel } from '@storybook/core/channels'; import type { PreparedStory, Renderer, StoryContext, StoryIndexEntry } from '@storybook/core/types'; import type { StoryStore } from '../../store'; @@ -26,6 +26,11 @@ const tick = () => new Promise((resolve) => setTimeout(resolve, 0)); window.location = { reload: vi.fn() } as any; +const mountSpy = vi.fn(async (context) => { + await context.renderToCanvas(); + return context.canvas; +}); + const buildStory = (overrides: Partial = {}): PreparedStory => ({ id: 'id', @@ -36,12 +41,7 @@ const buildStory = (overrides: Partial = {}): PreparedStory => applyBeforeEach: vi.fn(), unboundStoryFn: vi.fn(), playFunction: vi.fn(), - mount: vi.fn((context: StoryContext) => { - return async () => { - await context.renderToCanvas(); - return context.canvas; - }; - }), + mount: (context: StoryContext) => () => mountSpy(context), ...overrides, }) as any; @@ -53,6 +53,10 @@ const buildStore = (overrides: Partial> = {}): StoryStore { + vi.restoreAllMocks(); +}); + describe('StoryRender', () => { it('does run play function if passed autoplay=true', async () => { const story = buildStory(); @@ -133,12 +137,7 @@ describe('StoryRender', () => { }); it('calls mount if play function does not destructure mount', async () => { - const actualMount = vi.fn(async (context) => { - await context.renderToCanvas(); - return {}; - }); const story = buildStory({ - mount: (context) => () => actualMount(context) as any, playFunction: () => {}, }); const render = new StoryRender( @@ -153,23 +152,21 @@ describe('StoryRender', () => { ); await render.renderToElement({} as any); - expect(actualMount).toHaveBeenCalled(); + expect(mountSpy).toHaveBeenCalledOnce(); }); - it('does not call mount if play function destructures mount', async () => { - const actualMount = vi.fn(async (context) => { - await context.renderToCanvas(); - return context.canvas; - }); + it('errors if play function calls mount without destructuring', async () => { const story = buildStory({ - mount: (context) => () => actualMount(context) as any, - playFunction: ({ mount }) => {}, + playFunction: async (context) => { + await context.mount(); + }, }); + const view = { showException: vi.fn() }; const render = new StoryRender( new Channel({}), buildStore(), vi.fn() as any, - {} as any, + view as any, entry.id, 'story', { autoplay: true }, @@ -177,18 +174,13 @@ describe('StoryRender', () => { ); await render.renderToElement({} as any); - expect(actualMount).not.toHaveBeenCalled(); + expect(view.showException).toHaveBeenCalled(); }); - it('errors if play function calls mount without destructuring', async () => { - const actualMount = vi.fn(async (context) => { - await context.renderToCanvas(); - return {}; - }); + it('errors if play function destructures mount but does not call it', async () => { const story = buildStory({ - mount: (context) => () => actualMount(context) as any, - playFunction: async (context) => { - await context.mount(); + playFunction: async ({ mount }) => { + // forget to call mount }, }); const view = { showException: vi.fn() }; @@ -210,13 +202,15 @@ describe('StoryRender', () => { it('enters rendering phase during play if play function calls mount', async () => { const actualMount = vi.fn(async (context) => { await context.renderToCanvas(); - return {}; + expect(render.phase).toBe('rendering'); + return context.canvas; }); const story = buildStory({ mount: (context) => () => actualMount(context) as any, - playFunction: ({ mount }) => { + playFunction: async ({ mount }) => { + expect(render.phase).toBe('loading'); + await mount(); expect(render.phase).toBe('playing'); - mount(); }, }); const render = new StoryRender( diff --git a/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts b/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts index f26ab6663fcc..3a9e7bdb8943 100644 --- a/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts +++ b/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts @@ -9,7 +9,7 @@ import type { StoryStore } from '../../store'; import type { Render, RenderType } from './Render'; import { PREPARE_ABORTED } from './Render'; import { mountDestructured } from './mount-utils'; -import { MountMustBeDestructuredError } from '@storybook/core-events/preview-errors'; +import { MountMustBeDestructuredError, NoStoryMountedError } from '@storybook/core/preview-errors'; import type { Canvas, @@ -92,12 +92,19 @@ export class StoryRender implements Render Promise) { this.phase = phase; this.channel.emit(STORY_RENDER_PHASE_CHANGED, { newPhase: this.phase, storyId: this.id }); - if (phaseFn) await phaseFn(); + if (phaseFn) { + await phaseFn(); + this.checkIfAborted(signal); + } + } + private checkIfAborted(signal: AbortSignal): boolean { if (signal.aborted) { this.phase = 'aborted'; this.channel.emit(STORY_RENDER_PHASE_CHANGED, { newPhase: this.phase, storyId: this.id }); + return true; } + return false; } async prepare() { @@ -196,22 +203,28 @@ export class StoryRender implements Render runStep(label, play, context), context: null!, canvas: {} as Canvas, - mount: null!, renderToCanvas: async () => { + const teardown = await this.renderToScreen(renderContext, canvasElement); + this.teardownRender = teardown || (() => {}); + mounted = true; + }, + // The story provides (set in a renderer) a mount function that is a higher order function + // (context) => (...args) => Canvas + // + // Before assigning it to the context, we resolve the context dependency, + // so that a user can just call it as await mount(...args) in their play function. + mount: async (...args) => { + let mountReturn: Awaited> = null!; await this.runPhase(abortSignal, 'rendering', async () => { - const teardown = await this.renderToScreen(renderContext, canvasElement); - this.teardownRender = teardown || (() => {}); - mounted = true; + mountReturn = await story.mount(context)(...args); }); - - if (isMountDestructured) { - // put the phase back to playing if mount is used inside a play function - await this.runPhase(abortSignal, 'playing', async () => {}); - } + // start playing phase if mount is used inside a play function + if (isMountDestructured) await this.runPhase(abortSignal, 'playing'); + return mountReturn; }, }; + context.context = context; - context.mount = this.story.mount(context); const renderContext: RenderContext = { componentId, @@ -241,12 +254,10 @@ export class StoryRender implements Render { - const cleanupCallbacks = await applyBeforeEach(context); - this.store.addCleanupCallbacks(story, cleanupCallbacks); - }); + const cleanupCallbacks = await applyBeforeEach(context); + this.store.addCleanupCallbacks(story, cleanupCallbacks); - if (abortSignal.aborted) return; + if (this.checkIfAborted(abortSignal)) return; if (!mounted && !isMountDestructured) { await context.mount(); @@ -272,10 +283,17 @@ export class StoryRender implements Render { throw new MountMustBeDestructuredError({ playFunction: playFunction.toString() }); }; - } - await this.runPhase(abortSignal, 'playing', async () => { + await this.runPhase(abortSignal, 'playing', async () => playFunction(context)); + } else { + // when mount is used the playing phase will start later, right after mount is called in the play function await playFunction(context); - }); + } + + if (!mounted) { + throw new NoStoryMountedError(); + } + this.checkIfAborted(abortSignal); + if (!ignoreUnhandledErrors && unhandledErrors.size > 0) { await this.runPhase(abortSignal, 'errored'); } else { @@ -300,9 +318,9 @@ export class StoryRender implements Render - this.channel.emit(STORY_RENDERED, id) - ); + await this.runPhase(abortSignal, 'completed', async () => { + this.channel.emit(STORY_RENDERED, id); + }); } catch (err) { this.phase = 'errored'; this.callbacks.showException(err as Error); diff --git a/code/core/src/preview-errors.ts b/code/core/src/preview-errors.ts index 115f39556291..f8e820b60c30 100644 --- a/code/core/src/preview-errors.ts +++ b/code/core/src/preview-errors.ts @@ -295,6 +295,29 @@ export class TestingLibraryMustBeConfiguredError extends StorybookError { } } +export class NoStoryMountedError extends StorybookError { + readonly category = Category.PREVIEW_API; + + readonly code = 14; + + template() { + return dedent` + No story is mounted in your story. + + This usually occurs when you destructure mount in the play function, but forgot to call it. + + For example: + + async play({ mount, canvasElement }) { + // đŸ‘ˆmount should be called: await mount(); + const canvas = within(canvasElement); + const button = await canvas.findByRole('button'); + await userEvent.click(button); + }; + `; + } +} + export class NextJsSharpError extends StorybookError { readonly category = Category.FRAMEWORK_NEXTJS; From 771b92b7042e67cbdc0b37ca81386fce0e48706b Mon Sep 17 00:00:00 2001 From: Kasper Peulen Date: Wed, 3 Jul 2024 12:35:26 +0200 Subject: [PATCH 2/5] Add extra test --- .../preview-web/render/StoryRender.test.ts | 21 +++++++++++++++++++ .../modules/preview-web/render/StoryRender.ts | 6 +++--- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/code/core/src/preview-api/modules/preview-web/render/StoryRender.test.ts b/code/core/src/preview-api/modules/preview-web/render/StoryRender.test.ts index dce11cac9ad8..6ac9219cecd9 100644 --- a/code/core/src/preview-api/modules/preview-web/render/StoryRender.test.ts +++ b/code/core/src/preview-api/modules/preview-web/render/StoryRender.test.ts @@ -155,6 +155,27 @@ describe('StoryRender', () => { expect(mountSpy).toHaveBeenCalledOnce(); }); + it('does not call mount twice if mount called in play function', async () => { + const story = buildStory({ + playFunction: async ({ mount }) => { + await mount(); + }, + }); + const render = new StoryRender( + new Channel({}), + buildStore(), + vi.fn() as any, + {} as any, + entry.id, + 'story', + { autoplay: true }, + story + ); + + await render.renderToElement({} as any); + expect(mountSpy).toHaveBeenCalledOnce(); + }); + it('errors if play function calls mount without destructuring', async () => { const story = buildStory({ playFunction: async (context) => { diff --git a/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts b/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts index 3a9e7bdb8943..b467cecf438f 100644 --- a/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts +++ b/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts @@ -318,9 +318,9 @@ export class StoryRender implements Render { - this.channel.emit(STORY_RENDERED, id); - }); + await this.runPhase(abortSignal, 'completed', async () => + this.channel.emit(STORY_RENDERED, id) + ); } catch (err) { this.phase = 'errored'; this.callbacks.showException(err as Error); From a96a4f1889f80498a401c42a1e0fa32fb85f1326 Mon Sep 17 00:00:00 2001 From: Kasper Peulen Date: Wed, 3 Jul 2024 13:35:32 +0200 Subject: [PATCH 3/5] Remove loading screen at the right time --- .../modules/preview-web/PreviewWithSelection.tsx | 7 ++----- .../preview-api/modules/preview-web/render/StoryRender.ts | 6 +++++- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/code/core/src/preview-api/modules/preview-web/PreviewWithSelection.tsx b/code/core/src/preview-api/modules/preview-web/PreviewWithSelection.tsx index a0b98419f11e..09b890ffafa1 100644 --- a/code/core/src/preview-api/modules/preview-web/PreviewWithSelection.tsx +++ b/code/core/src/preview-api/modules/preview-web/PreviewWithSelection.tsx @@ -308,11 +308,7 @@ export class PreviewWithSelection extends Preview( this.channel, this.storyStoreValue, - (...args: Parameters) => { - // At the start of renderToCanvas we make the story visible (see note in WebView) - this.view.showStoryDuringRender(); - return renderToCanvas(...args); - }, + renderToCanvas, this.mainStoryCallbacks(storyId), storyId, 'story' @@ -434,6 +430,7 @@ export class PreviewWithSelection extends Preview this.view.showStoryDuringRender(), showMain: () => this.view.showMain(), showError: (err: { title: string; description: string }) => this.renderError(storyId, err), showException: (err: Error) => this.renderException(storyId, err), diff --git a/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts b/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts index b467cecf438f..4910455e96d4 100644 --- a/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts +++ b/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts @@ -71,7 +71,7 @@ export class StoryRender implements Render, private renderToScreen: RenderToCanvas, - private callbacks: RenderContextCallbacks, + private callbacks: RenderContextCallbacks & { showStoryDuringRender: () => void }, public id: StoryId, public viewMode: StoryContext['viewMode'], public renderOptions: StoryRenderOptions = { autoplay: true, forceInitialArgs: false }, @@ -214,6 +214,7 @@ export class StoryRender implements Render { + this.callbacks.showStoryDuringRender(); let mountReturn: Awaited> = null!; await this.runPhase(abortSignal, 'rendering', async () => { mountReturn = await story.mount(context)(...args); @@ -300,6 +301,9 @@ export class StoryRender implements Render { this.channel.emit(PLAY_FUNCTION_THREW_EXCEPTION, serializeError(error)); }); From b34500df2b01bc326716a03141593d021d90273d Mon Sep 17 00:00:00 2001 From: Kasper Peulen Date: Wed, 3 Jul 2024 16:03:19 +0200 Subject: [PATCH 4/5] Fix build error --- .../preview-api/modules/preview-web/render/StoryRender.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts b/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts index 4910455e96d4..25f20777ee39 100644 --- a/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts +++ b/code/core/src/preview-api/modules/preview-web/render/StoryRender.ts @@ -71,7 +71,7 @@ export class StoryRender implements Render, private renderToScreen: RenderToCanvas, - private callbacks: RenderContextCallbacks & { showStoryDuringRender: () => void }, + private callbacks: RenderContextCallbacks & { showStoryDuringRender?: () => void }, public id: StoryId, public viewMode: StoryContext['viewMode'], public renderOptions: StoryRenderOptions = { autoplay: true, forceInitialArgs: false }, @@ -214,7 +214,7 @@ export class StoryRender implements Render { - this.callbacks.showStoryDuringRender(); + this.callbacks.showStoryDuringRender?.(); let mountReturn: Awaited> = null!; await this.runPhase(abortSignal, 'rendering', async () => { mountReturn = await story.mount(context)(...args); @@ -302,7 +302,7 @@ export class StoryRender implements Render { this.channel.emit(PLAY_FUNCTION_THREW_EXCEPTION, serializeError(error)); From de38fa60ace5db0213a1e05ddd2e20cf98aae079 Mon Sep 17 00:00:00 2001 From: Kasper Peulen Date: Wed, 3 Jul 2024 17:18:53 +0200 Subject: [PATCH 5/5] Adjust tests --- code/core/src/preview-api/modules/preview-web/PreviewWeb.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/code/core/src/preview-api/modules/preview-web/PreviewWeb.test.ts b/code/core/src/preview-api/modules/preview-web/PreviewWeb.test.ts index 16b1831e8e23..ff4a8b67c443 100644 --- a/code/core/src/preview-api/modules/preview-web/PreviewWeb.test.ts +++ b/code/core/src/preview-api/modules/preview-web/PreviewWeb.test.ts @@ -3556,6 +3556,7 @@ describe('PreviewWeb', () => { await (preview.storyStore as StoryStore)?.loadStory({ storyId: 'component-one--b', }), + {} as any, {} as any ); await waitForRenderPhase('playing');