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

Core: Introduce run over play in portable stories, and revert back play changes of 8.2 #28764

Merged
merged 19 commits into from
Jul 31, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
339b541
Document `Story.run()` instead of `Story.play()`
kylegach Jul 30, 2024
653324d
Introduce run over play in portable stories, and revert back play cha…
kasperpeulen Jul 31, 2024
d85f877
Merge remote-tracking branch 'refs/remotes/origin/portable-stories-pl…
kasperpeulen Jul 31, 2024
25ab52b
Fix test
kasperpeulen Jul 31, 2024
7f86411
Fix BS
kasperpeulen Jul 31, 2024
bb34a32
Discard changes to test-storybooks/portable-stories-kitchen-sink/svel…
yannbf Jul 31, 2024
e9c18ff
Discard changes to test-storybooks/portable-stories-kitchen-sink/vue3…
yannbf Jul 31, 2024
d7d9092
Discard changes to test-storybooks/portable-stories-kitchen-sink/next…
yannbf Jul 31, 2024
63d3aec
Discard changes to test-storybooks/portable-stories-kitchen-sink/reac…
yannbf Jul 31, 2024
a53df3a
Discard changes to test-storybooks/portable-stories-kitchen-sink/reac…
yannbf Jul 31, 2024
f9605a7
Update docs/api/portable-stories/portable-stories-jest.mdx
kasperpeulen Jul 31, 2024
a762069
Update docs/api/portable-stories/portable-stories-jest.mdx
kasperpeulen Jul 31, 2024
b1acd94
Update docs/api/portable-stories/portable-stories-jest.mdx
kasperpeulen Jul 31, 2024
a6eea4b
Update docs/api/portable-stories/portable-stories-vitest.mdx
kasperpeulen Jul 31, 2024
fa39958
Fix story pipeline image
kasperpeulen Jul 31, 2024
f139626
Address feedback
kasperpeulen Jul 31, 2024
9d2af7c
Merge remote-tracking branch 'origin/kasper/introduce-run' into kaspe…
kasperpeulen Jul 31, 2024
89eddca
Update docs/api/portable-stories/portable-stories-vitest.mdx
kasperpeulen Jul 31, 2024
2772623
Update docs/api/portable-stories/portable-stories-vitest.mdx
kasperpeulen Jul 31, 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
55 changes: 23 additions & 32 deletions code/core/src/preview-api/modules/store/csf/portable-stories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,25 +89,8 @@ export function composeStory<TRenderer extends Renderer = Renderer, TArgs extend
normalizedComponentAnnotations
);

// TODO: Remove this in 9.0
// We can only use the renderToCanvas definition of the default config when testingLibraryRender is set
// This makes sure, that when the user doesn't do this, and doesn't provide its own renderToCanvas definition,
// we fall back to the < 8.1 behavior of the play function.

const fallback =
defaultConfig &&
!globalProjectAnnotations?.testingLibraryRender &&
!projectAnnotations?.testingLibraryRender;

const normalizedProjectAnnotations = normalizeProjectAnnotations<TRenderer>(
composeConfigs([
{
...defaultConfig,
renderToCanvas: fallback ? undefined : defaultConfig?.renderToCanvas,
},
globalProjectAnnotations,
projectAnnotations ?? {},
])
composeConfigs([defaultConfig ?? {}, globalProjectAnnotations, projectAnnotations ?? {}])
);

const story = prepareStory<TRenderer>(
Expand All @@ -130,7 +113,7 @@ export function composeStory<TRenderer extends Renderer = Renderer, TArgs extend
loaded: {},
kasperpeulen marked this conversation as resolved.
Show resolved Hide resolved
abortSignal: new AbortController().signal,
step: (label, play) => story.runStep(label, play, context),
canvasElement: globalThis?.document?.body,
canvasElement: null!,
canvas: {} as Canvas,
...story,
context: null!,
Expand All @@ -149,6 +132,7 @@ export function composeStory<TRenderer extends Renderer = Renderer, TArgs extend
id: story.id,
name: story.name,
tags: story.tags,
showMain: () => {},
showError: (error) => {},
showException: (error) => {},
forceRemount: true,
Expand All @@ -171,28 +155,23 @@ export function composeStory<TRenderer extends Renderer = Renderer, TArgs extend

let loadedContext: StoryContext<TRenderer> | undefined;

// TODO: Remove in 9.0
const backwardsCompatiblePlay = async (
extraContext?: Partial<StoryContext<TRenderer, Partial<TArgs>>>
) => {
const play = async (extraContext?: Partial<StoryContext<TRenderer, Partial<TArgs>>>) => {
const context = initializeContext();
kasperpeulen marked this conversation as resolved.
Show resolved Hide resolved
context.canvasElement ??= globalThis?.document?.body;
if (loadedContext) {
context.loaded = loadedContext.loaded;
}
Object.assign(context, extraContext);
return story.playFunction!(context);
};
const newPlay = (extraContext?: Partial<StoryContext<TRenderer, Partial<TArgs>>>) => {

const run = (extraContext?: Partial<StoryContext<TRenderer, Partial<TArgs>>>) => {
const context = initializeContext();
Object.assign(context, extraContext);
return playStory(story, context);
return runStory(story, context);
};
const playFunction =
!story.renderToCanvas && story.playFunction
? backwardsCompatiblePlay
: !story.renderToCanvas && !story.playFunction
? undefined
: newPlay;

const playFunction = story.playFunction ? play : undefined;

const composedStory: ComposedStoryFn<TRenderer, Partial<TArgs>> = Object.assign(
function storyFn(extraArgs?: Partial<TArgs>) {
Expand Down Expand Up @@ -226,6 +205,7 @@ export function composeStory<TRenderer extends Renderer = Renderer, TArgs extend
parameters: story.parameters as Parameters,
argTypes: story.argTypes as StrictArgTypes<TArgs>,
play: playFunction!,
run,
tags: story.tags,
}
);
Expand Down Expand Up @@ -325,13 +305,24 @@ export function createPlaywrightTest<TFixture extends { extend: any }>(

// TODO At some point this function should live in prepareStory and become the core of StoryRender.render as well.
// Will make a follow up PR for that
async function playStory<TRenderer extends Renderer>(
async function runStory<TRenderer extends Renderer>(
kasperpeulen marked this conversation as resolved.
Show resolved Hide resolved
story: PreparedStory<TRenderer>,
context: StoryContext<TRenderer>
) {
for (const callback of [...cleanups].reverse()) await callback();
cleanups.length = 0;

if (!context.canvasElement) {
const container = document.createElement('div');
globalThis?.document?.body?.appendChild(container);
context.canvasElement = container;
cleanups.push(() => {
if (globalThis?.document?.body?.contains(container)) {
globalThis?.document?.body?.removeChild(container);
}
});
}

context.loaded = await story.applyLoaders(context);
if (context.abortSignal.aborted) return;

Expand Down
3 changes: 2 additions & 1 deletion code/core/src/types/modules/composedStory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ export type ComposedStoryFn<
> = PartialArgsStoryFn<TRenderer, TArgs> & {
args: TArgs;
id: StoryId;
play: (context?: Partial<StoryContext<TRenderer, Partial<TArgs>>>) => Promise<void>;
play?: (context?: Partial<StoryContext<TRenderer, Partial<TArgs>>>) => Promise<void>;
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Making play optional might cause issues if existing code relies on it being present.

run: (context?: Partial<StoryContext<TRenderer, Partial<TArgs>>>) => Promise<void>;
kasperpeulen marked this conversation as resolved.
Show resolved Hide resolved
load: () => Promise<void>;
storyName: string;
parameters: Parameters;
Expand Down
10 changes: 5 additions & 5 deletions code/renderers/react/src/__test__/portable-stories.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ describe('renders', () => {
});

it('should render component mounted in play function', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Ensure run function provides the same functionality as play to avoid test failures.

await MountInPlayFunction.play();
await MountInPlayFunction.run();

expect(screen.getByTestId('spy-data').textContent).toEqual('mockFn return value');
expect(screen.getByTestId('loaded-data').textContent).toEqual('loaded data');
Expand All @@ -65,7 +65,7 @@ describe('renders', () => {
expect(getByTestId('spy-data').textContent).toEqual('mockFn return value');
expect(getByTestId('loaded-data').textContent).toEqual('loaded data');
// spy assertions happen in the play function and should work
await LoaderStory.play!();
await LoaderStory.run!();
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Verify that LoaderStory.run correctly handles the data composition and spy assertions.

});
});

Expand Down Expand Up @@ -125,7 +125,7 @@ describe('CSF3', () => {

it('renders with play function without canvas element', async () => {
const CSF3InputFieldFilled = composeStory(stories.CSF3InputFieldFilled, stories.default);
await CSF3InputFieldFilled.play();
await CSF3InputFieldFilled.run();
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Confirm that CSF3InputFieldFilled.run correctly initializes the input field value.


const input = screen.getByTestId('input') as HTMLInputElement;
expect(input.value).toEqual('Hello world!');
Expand All @@ -138,7 +138,7 @@ describe('CSF3', () => {
console.log(div.tagName);
document.body.appendChild(div);

await CSF3InputFieldFilled.play({ canvasElement: div });
await CSF3InputFieldFilled.run({ canvasElement: div });
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Check if CSF3InputFieldFilled.run correctly interacts with the canvas element.


const input = screen.getByTestId('input') as HTMLInputElement;
expect(input.value).toEqual('Hello world!');
Expand Down Expand Up @@ -185,6 +185,6 @@ const testCases = Object.values(composeStories(stories)).map(
);
it.each(testCases)('Renders %s story', async (_storyName, Story) => {
if (_storyName === 'CSF2StoryWithLocale') return;
await Story.play();
await Story.run();
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Ensure all stories in testCases are correctly rendered using the run function.

expect(document.body).toMatchSnapshot();
});
15 changes: 10 additions & 5 deletions code/renderers/react/src/portable-stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,16 @@ export function setProjectAnnotations(
// This will not be necessary once we have auto preset loading
export const INTERNAL_DEFAULT_PROJECT_ANNOTATIONS: ProjectAnnotations<ReactRenderer> = {
...reactProjectAnnotations,
renderToCanvas: ({
storyContext: { context, unboundStoryFn: Story, testingLibraryRender: render, canvasElement },
}) => {
if (render == null) throw new TestingLibraryMustBeConfiguredError();
const { unmount } = render(<Story {...context} />, { baseElement: context.canvasElement });
renderToCanvas: (renderContext, canvasElement) => {
if (renderContext.storyContext.testingLibraryRender == null) {
throw new TestingLibraryMustBeConfiguredError();
// Enable for 8.3
Comment on lines +49 to +51
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Ensure testingLibraryRender is always configured to avoid runtime errors.

// return reactProjectAnnotations.renderToCanvas(renderContext, canvasElement);
}
const {
storyContext: { context, unboundStoryFn: Story, testingLibraryRender: render },
} = renderContext;
const { unmount } = render(<Story {...context} />, { container: context.canvasElement });
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: Consider using a more descriptive variable name than context.canvasElement for clarity.

return unmount;
},
};
Expand Down
Loading