Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test: Add mount property to the story context #28383

Merged
merged 38 commits into from
Jul 3, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
3eb587f
Add a mount property to the context, so that the user can do stuff be…
kasperpeulen Jun 28, 2024
6da574a
Merge branch 'refs/heads/kasper/canvas' into kasper/mount
kasperpeulen Jun 28, 2024
ea68fe9
Merge branch 'kasper/canvas' into kasper/mount
kasperpeulen Jun 28, 2024
5ba7b39
Fix snapshots
kasperpeulen Jun 28, 2024
15e354e
Merge remote-tracking branch 'origin/kasper/mount' into kasper/mount
kasperpeulen Jun 28, 2024
85b91bb
Cleanup
kasperpeulen Jun 28, 2024
fa74a4f
Resolve some comments
kasperpeulen Jun 28, 2024
5bb7da9
Fix angular types in test
kasperpeulen Jun 28, 2024
540083c
Merge branch 'refs/heads/kasper/canvas' into kasper/mount
kasperpeulen Jun 28, 2024
5506e78
Add comment
kasperpeulen Jun 28, 2024
c38596b
Merge branch 'kasper/canvas' into kasper/mount
kasperpeulen Jul 1, 2024
835621d
Add a commented out test
kasperpeulen Jul 1, 2024
7a3bbff
Always create a fresh context when playing or loading a story
kasperpeulen Jul 1, 2024
d6f047b
add tests for legacy format of portable stories
yannbf Jul 1, 2024
362ec4d
Add long definition test
kasperpeulen Jul 1, 2024
254102f
Use utility
kasperpeulen Jul 1, 2024
b6c4c8c
Address feedback
kasperpeulen Jul 1, 2024
a5b50d9
Rename errors
kasperpeulen Jul 1, 2024
a32c0cd
Make portable stories play backwards compatible
kasperpeulen Jul 1, 2024
e8479d3
Call prepare context at the right time
kasperpeulen Jul 1, 2024
7db654a
Fix all unit tests
kasperpeulen Jul 2, 2024
fa7aa1f
Fix all unit tests
kasperpeulen Jul 2, 2024
8b23c6c
Merge remote-tracking branch 'origin/kasper/mount' into kasper/mount
kasperpeulen Jul 2, 2024
6193e85
Merge branch 'kasper/canvas' into kasper/mount
kasperpeulen Jul 2, 2024
ab925e8
Merge branch 'kasper/canvas' into kasper/mount
kasperpeulen Jul 2, 2024
8870451
Fix CPC imports
kasperpeulen Jul 2, 2024
9b0ec09
Use @storybook/csf 0.1.11
kasperpeulen Jul 2, 2024
840eef5
Refactor tests slightly
tmeasday Jul 2, 2024
771af17
Add tests for `storyRender`
tmeasday Jul 2, 2024
41b39be
Fix storybook/core reference
kasperpeulen Jul 2, 2024
e557116
Fix CPC imports
kasperpeulen Jul 2, 2024
2945163
Fix type errors
kasperpeulen Jul 2, 2024
db487f6
Add a test that we force remount destructuring stories on args change
tmeasday Jul 2, 2024
c7284bf
Put back portable story playwright implementation
kasperpeulen Jul 2, 2024
fb3f6cd
keepNames to false to restore previous compiling behavior
kasperpeulen Jul 2, 2024
d2b58e0
Fix last issues 🤞
kasperpeulen Jul 2, 2024
c0084fb
Fix portable stories test
kasperpeulen Jul 2, 2024
9678d6f
Put phase back to playing after rendering in a mount story
kasperpeulen Jul 2, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion code/addons/links/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@
"prep": "node --loader ../../../scripts/node_modules/esbuild-register/loader.js -r ../../../scripts/node_modules/esbuild-register/register.js ../../../scripts/prepare/addon-bundle.ts"
},
"dependencies": {
"@storybook/csf": "0.1.10--canary.99.ba76785.0",
"@storybook/csf": "0.1.10--canary.100.877a297.0",
"@storybook/global": "^5.0.0",
"ts-dedent": "^2.0.0"
},
Expand Down
2 changes: 1 addition & 1 deletion code/lib/codemod/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@
"@babel/core": "^7.24.4",
"@babel/preset-env": "^7.24.4",
"@babel/types": "^7.24.0",
"@storybook/csf": "0.1.10--canary.99.ba76785.0",
"@storybook/csf": "0.1.10--canary.100.877a297.0",
"@storybook/csf-tools": "workspace:*",
"@storybook/node-logger": "workspace:*",
"@storybook/types": "workspace:*",
Expand Down
2 changes: 1 addition & 1 deletion code/lib/core-events/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@
"prep": "node --loader ../../../scripts/node_modules/esbuild-register/loader.js -r ../../../scripts/node_modules/esbuild-register/register.js ../../../scripts/prepare/bundle.ts"
},
"dependencies": {
"@storybook/csf": "0.1.10--canary.99.ba76785.0",
"@storybook/csf": "0.1.10--canary.100.877a297.0",
"ts-dedent": "^2.0.0"
},
"devDependencies": {
Expand Down
57 changes: 57 additions & 0 deletions code/lib/core-events/src/errors/preview-errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,63 @@ export class StoryStoreAccessedBeforeInitializationError extends StorybookError
}
}

export class MountMustBeDestructured extends StorybookError {
kasperpeulen marked this conversation as resolved.
Show resolved Hide resolved
readonly category = Category.PREVIEW_API;

readonly code = 12;

constructor(public data: { playFunction: string }) {
super();
}

template() {
return dedent`
To use mount in the play function, you must use object destructuring, e.g. play: ({ mount }) => {}.

Instead received:
${this.data.playFunction}
`;
}
}

export class TestingLibraryMustBeConfigured extends StorybookError {
kasperpeulen marked this conversation as resolved.
Show resolved Hide resolved
readonly category = Category.PREVIEW_API;

readonly code = 13;

template() {
return dedent`
You must configure testingLibraryRender to use play in portable stories.

import { render } from '@testing-library/[renderer]';

setProjectAnnotations({
testingLibraryRender: render,
});

For other testing renderers, you can configure renderToCanvas:

import { render } from 'your-renderer';

setProjectAnnotations({
renderToCanvas: ({ storyFn }) => {
const Story = storyFn();

// Svelte
render(Story.Component, Story.props);

// Vue
render(Story);

// or for React
render(<Story/>);
},
});

`;
}
kasperpeulen marked this conversation as resolved.
Show resolved Hide resolved
}

export class NextJsSharpError extends StorybookError {
readonly category = Category.FRAMEWORK_NEXTJS;

Expand Down
2 changes: 1 addition & 1 deletion code/lib/core-server/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@
"@storybook/channels": "workspace:*",
"@storybook/core-common": "workspace:*",
"@storybook/core-events": "workspace:*",
"@storybook/csf": "0.1.10--canary.99.ba76785.0",
"@storybook/csf": "0.1.10--canary.100.877a297.0",
"@storybook/csf-tools": "workspace:*",
"@storybook/docs-mdx": "3.1.0-next.0",
"@storybook/global": "^5.0.0",
Expand Down
2 changes: 1 addition & 1 deletion code/lib/csf-tools/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
"@babel/parser": "^7.24.4",
"@babel/traverse": "^7.24.1",
"@babel/types": "^7.24.0",
"@storybook/csf": "0.1.10--canary.99.ba76785.0",
"@storybook/csf": "0.1.10--canary.100.877a297.0",
"@storybook/types": "workspace:*",
"fs-extra": "^11.1.0",
"recast": "^0.23.5",
Expand Down
2 changes: 1 addition & 1 deletion code/lib/manager-api/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
"@storybook/channels": "workspace:*",
"@storybook/client-logger": "workspace:*",
"@storybook/core-events": "workspace:*",
"@storybook/csf": "0.1.10--canary.99.ba76785.0",
"@storybook/csf": "0.1.10--canary.100.877a297.0",
"@storybook/global": "^5.0.0",
"@storybook/icons": "^1.2.5",
"@storybook/router": "workspace:*",
Expand Down
2 changes: 1 addition & 1 deletion code/lib/preview-api/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
"@storybook/channels": "workspace:*",
"@storybook/client-logger": "workspace:*",
"@storybook/core-events": "workspace:*",
"@storybook/csf": "0.1.10--canary.99.ba76785.0",
"@storybook/csf": "0.1.10--canary.100.877a297.0",
"@storybook/global": "^5.0.0",
"@storybook/types": "workspace:*",
"@types/qs": "^6.9.5",
Expand Down
5 changes: 4 additions & 1 deletion code/lib/preview-api/src/modules/preview-web/Preview.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ import { StoryStore } from '../../store';
import { StoryRender } from './render/StoryRender';
import type { CsfDocsRender } from './render/CsfDocsRender';
import type { MdxDocsRender } from './render/MdxDocsRender';
import { mountDestructured } from './render/mount-utils';

const { fetch } = global;

Expand Down Expand Up @@ -295,7 +296,9 @@ export class Preview<TRenderer extends Renderer> {
await Promise.all(
this.storyRenders
.filter((r) => r.id === storyId && !r.renderOptions.forceInitialArgs)
.map((r) => r.rerender())
.map((r) =>
r.story && mountDestructured(r.story.playFunction) ? r.remount() : r.rerender()
kasperpeulen marked this conversation as resolved.
Show resolved Hide resolved
)
kasperpeulen marked this conversation as resolved.
Show resolved Hide resolved
);

this.channel.emit(STORY_ARGS_UPDATED, {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3626,6 +3626,7 @@ describe('PreviewWeb', () => {
"dev",
"test",
],
"testingLibraryRender": undefined,
"title": "Component One",
},
"component-one--b": {
Expand Down Expand Up @@ -3674,6 +3675,7 @@ describe('PreviewWeb', () => {
"dev",
"test",
],
"testingLibraryRender": undefined,
"title": "Component One",
},
"component-one--e": {
Expand All @@ -3700,6 +3702,7 @@ describe('PreviewWeb', () => {
"dev",
"test",
],
"testingLibraryRender": undefined,
"title": "Component One",
},
"component-two--c": {
Expand Down Expand Up @@ -3736,6 +3739,7 @@ describe('PreviewWeb', () => {
"dev",
"test",
],
"testingLibraryRender": undefined,
"title": "Component Two",
},
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -471,17 +471,9 @@ export class PreviewWithSelection<TRenderer extends Renderer> extends Preview<TR
this.channel.emit(STORY_THREW_EXCEPTION, { name, message, stack });
this.channel.emit(STORY_RENDER_PHASE_CHANGED, { newPhase: 'errored', storyId });

// Ignored exceptions exist for control flow purposes, and are typically handled elsewhere.
//
// FIXME: Should be '=== IGNORED_EXCEPTION', but currently the object
// is coming from two different bundles (index.js vs index.mjs)
//
// https://github.com/storybookjs/storybook/issues/19321
if (!error.message?.startsWith('ignoredException')) {
this.view.showErrorDisplay(error);
logger.error(`Error rendering story '${storyId}':`);
logger.error(error);
}
this.view.showErrorDisplay(error);
logger.error(`Error rendering story '${storyId}':`);
logger.error(error);
kasperpeulen marked this conversation as resolved.
Show resolved Hide resolved
}

// renderError is used by the various app layers to inform the user they have done something
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
// @vitest-environment happy-dom
import { describe, it, expect, vi } from 'vitest';
import { Channel } from '@storybook/channels';
import type { Renderer, StoryIndexEntry } from '@storybook/types';
import type { Renderer, StoryContext, StoryIndexEntry } from '@storybook/types';
import type { StoryStore } from '../../store';
import { PREPARE_ABORTED } from './Render';

Expand All @@ -26,6 +26,13 @@ const tick = () => new Promise((resolve) => setTimeout(resolve, 0));

window.location = { reload: vi.fn() } as any;

const mount = (context: StoryContext) => {
return async () => {
await context.renderToCanvas();
return {};
};
};

describe('StoryRender', () => {
it('does run play function if passed autoplay=true', async () => {
const story = {
Expand All @@ -38,6 +45,7 @@ describe('StoryRender', () => {
unboundStoryFn: vi.fn(),
playFunction: vi.fn(),
prepareContext: vi.fn(),
mount,
};

const render = new StoryRender(
Expand Down Expand Up @@ -66,6 +74,7 @@ describe('StoryRender', () => {
unboundStoryFn: vi.fn(),
playFunction: vi.fn(),
prepareContext: vi.fn(),
mount,
};

const render = new StoryRender(
Expand All @@ -86,6 +95,8 @@ describe('StoryRender', () => {
it('only rerenders once when triggered multiple times while pending', async () => {
// Arrange - setup StoryRender and async gate blocking applyLoaders
const [loaderGate, openLoaderGate] = createGate();
const renderToScreen = vi.fn();

const story = {
id: 'id',
title: 'title',
Expand All @@ -96,13 +107,13 @@ describe('StoryRender', () => {
unboundStoryFn: vi.fn(),
playFunction: vi.fn(),
prepareContext: vi.fn(),
mount,
};
const store = {
getStoryContext: () => ({}),
cleanupStory: vi.fn(),
addCleanupCallbacks: vi.fn(),
};
const renderToScreen = vi.fn();
const render = new StoryRender(
new Channel({}),
store as any,
Expand Down Expand Up @@ -182,6 +193,7 @@ describe('StoryRender', () => {
unboundStoryFn: vi.fn(),
playFunction: vi.fn(),
prepareContext: vi.fn(),
mount,
};
const store = {
getStoryContext: () => ({}),
Expand Down Expand Up @@ -225,6 +237,7 @@ describe('StoryRender', () => {
unboundStoryFn: vi.fn(),
playFunction: vi.fn(),
prepareContext: vi.fn(),
mount,
};
const store = {
getStoryContext: () => ({}),
Expand Down Expand Up @@ -271,6 +284,7 @@ describe('StoryRender', () => {
unboundStoryFn: vi.fn(),
playFunction: vi.fn(() => playGate),
prepareContext: vi.fn(),
mount,
};
const store = {
getStoryContext: () => ({}),
Expand Down Expand Up @@ -317,6 +331,7 @@ describe('StoryRender', () => {
unboundStoryFn: vi.fn(),
playFunction: vi.fn(),
prepareContext: vi.fn(),
mount,
};
const store = {
getStoryContext: () => ({}),
Expand Down
27 changes: 23 additions & 4 deletions code/lib/preview-api/src/modules/preview-web/render/StoryRender.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import {
import type { StoryStore } from '../../store';
import type { Render, RenderType } from './Render';
import { PREPARE_ABORTED } from './Render';
import { getUsedProps } from './mount-utils';
import { MountMustBeDestructured } from '@storybook/core-events/preview-errors';

const { AbortController } = globalThis;

Expand Down Expand Up @@ -178,6 +180,8 @@ export class StoryRender<TRenderer extends Renderer> implements Render<TRenderer
// abort controller may be torn down (above) before we actually check the signal.
const abortSignal = (this.abortController as AbortController).signal;

let mounted = false;

try {
const context: StoryContext<TRenderer> = {
...this.storyContext(),
Expand All @@ -188,8 +192,17 @@ export class StoryRender<TRenderer extends Renderer> implements Render<TRenderer
step: (label, play) => runStep(label, play, context),
context: null!,
canvas: {},
mount: null!,
renderToCanvas: async () => {
await this.runPhase(abortSignal, 'rendering', async () => {
const teardown = await this.renderToScreen(renderContext, canvasElement);
this.teardownRender = teardown || (() => {});
mounted = true;
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why the runPhase code is up here, it seems like the wrong place? Shouldn't just be down on line 245, in flow with the rest of it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is that this is also called when calling mount from inside the play function and line 245 is only called, when mount is not used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see. I guess this control flow from StoryRender -> story.mount (in prepareStory) -> renderToCanvas, or StoryRender -> play -> context.mount -> renderToCanvas is pretty hard to understand off the bat.

Is there something we can do to make it easier to follow (either in code or just comments?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will refactor this bit in a new PR.

},
};
context.context = context;
context.mount = this.story.mount(context);

const renderContext: RenderContext<TRenderer> = {
componentId,
Expand Down Expand Up @@ -226,10 +239,11 @@ export class StoryRender<TRenderer extends Renderer> implements Render<TRenderer

if (abortSignal.aborted) return;

await this.runPhase(abortSignal, 'rendering', async () => {
const teardown = await this.renderToScreen(renderContext, canvasElement);
this.teardownRender = teardown || (() => {});
});
const mountDestructured = playFunction && getUsedProps(playFunction).includes('mount');
kasperpeulen marked this conversation as resolved.
Show resolved Hide resolved

if (!mounted && !mountDestructured) {
await context.mount();
}

this.notYetRendered = false;
if (abortSignal.aborted) return;
Expand All @@ -247,6 +261,11 @@ export class StoryRender<TRenderer extends Renderer> implements Render<TRenderer
window.addEventListener('unhandledrejection', onError);
this.disableKeyListeners = true;
try {
if (!mountDestructured) {
context.mount = async () => {
throw new MountMustBeDestructured({ playFunction: playFunction.toString() });
tmeasday marked this conversation as resolved.
Show resolved Hide resolved
};
}
await this.runPhase(abortSignal, 'playing', async () => {
await playFunction(context);
});
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { expect, test } from 'vitest';
import { getUsedProps } from './mount-utils';

const StoryWithContext = {
play: async (context: any) => {
console.log(context);
},
};

const StoryWitCanvasElement = {
play: async ({ canvasElement }: any) => {
console.log(canvasElement);
},
};

const MountStory = {
play: async ({ mount }: any) => {
await mount();
},
};

test('Detect destructure', () => {
expect(getUsedProps(StoryWithContext.play)).toMatchInlineSnapshot(`[]`);
expect(getUsedProps(StoryWitCanvasElement.play)).toMatchInlineSnapshot(`
[
"canvasElement",
]
`);

expect(getUsedProps(MountStory.play)).toMatchInlineSnapshot(`
[
"mount",
]
`);
});
kasperpeulen marked this conversation as resolved.
Show resolved Hide resolved
Loading