Documentation: Extendability and Re-usability of the `play` function

Foxhoundn commented 1 year ago

Describe the bug

Hi! 👋🏽

Most relevant people already know what this issue is about so I will try to keep it as short as I can.

Documentation

The initial issue originated from TypeScript errors when re-using the play function - in the Documentation it is stated that all you need to do is call another components' play function with the an object that contains { canvasElement }

This introduces a TypeScript error which says that other properties are missing from this object.

Type '{ canvasElement: HTMLElement; }' is missing the following properties from type 'StoryIdentifier': componentId, title, kind, id, and 3 more.ts(2345)

@ts-ignore fixed this error, without any impact that I could find
It was investigated first with @IanVS and we decided that the ...rest props should be passed as well, that fixed the issue for all cases and I created a PR where I changed the documentation & demos, all was good :) https://github.com/storybookjs/storybook/pull/21517

Re-usability

All wasn't good - as literally a day after a user neo0807 opened a new issue on Discord (https://discord.com/channels/486522875931656193/1083439468658299011) saying they have the same error when re-using the function in jest. The PR was already done so I made the following comment:

Well to be honest - I do not know why the whole context is required, it's how the play function is typed. Thinking about it more and seeing that if you @ts-ignore and just pass { canvasElement}, everything seems to work fine - so maybe a better solution would be to either make the other props optional, or create a new interface for the play function that has optional / partial props of the context.

One reason for this would be when you want to reuse play functions in unit test as one user did here - https://discord.com/channels/486522875931656193/1083439468658299011 - you have no way of providing the whole context to the play function, but you can specify the canvasElement (usually it's going to be document or something like that in a jest scenario). Atm if I as a user tried to reuse the play function in a test, I'd be very confused as to what exactly do I need to pass as the arguments.

There is no way to pass the whole context from within jest, a lot of the properties are internal storybook data that users have no knowledge about.

Extendability

Playing with the play (lol) function a lot this weekend I found that there is no way to:
1: Get the context (the second argument of a story) inside the play function - https://discord.com/channels/486522875931656193/1084581878574628905
2: Extend the play function with custom arguments that are not part of the component arguments - For example let's say you have 5 different buttons, with 5 different IDs, you write a ShouldClickButton play function that clicks a button with the provided ID

play: async (context, someCustomParamOrData) => {
   const canvas = within(context.canvasElement);
   await fireEvent.click(canvas.getByTestId(someCustomParamOrData));
   ...
}

play: async (context) => {
  await OtherComponent.play(context, 'button-2');
}

We could pass this as args sure, but then we are changing the actual interface of the component and it's props, which could lead bugs / inconsistencies. I think this could be solved with having the context available in the play function (1).

Tagging all the relevant people @IanVS, @jonniebigodes, @kylegach and @yannbf.

Please let me know if something is not clear and thanks for making an amazing product!

To Reproduce

This is directly from the 7.0 (rc) documentation and should reproduce the error.

/*
 * See https://storybook.js.org/docs/7.0/react/writing-stories/play-function#working-with-the-canvas
 * to learn more about using the canvasElement to query the DOM
 */
export const FirstStory = {
  play: async ({ canvasElement }) => {
    const canvas = within(canvasElement);

    userEvent.type(canvas.getByTestId('an-element'), 'example-value');
  },
};

export const SecondStory = {
  play: async ({ canvasElement }) => {
    const canvas = within(canvasElement);

    await userEvent.type(canvas.getByTestId('other-element'), 'another value');
  },
};

export const CombinedStories = {
  play: async ({ canvasElement }) => {
    const canvas = within(canvasElement);

    // Runs the FirstStory and Second story play function before running this story's play function
    await FirstStory.play({ canvasElement });
    await SecondStory.play({ canvasElement });
    await userEvent.type(canvas.getByTestId('another-element'), 'random value');
  },
};

System

Storybook 7.0.0-beta.59

Additional context

https://github.com/storybookjs/storybook/issues/21562

Foxhoundn commented 1 year ago

Update regarding:

- 1: Get the `context` (the second argument of a story) inside the play function - https://discord.com/channels/486522875931656193/1084581878574628905

I do not think this is necessary anymore because I've learned from @kylegach that I've been doing it wrong. I've been passing arguments to <Story in decorators as <Story arg1={value1} but instead it should be <Story args={{ arg1: value1.

The issue is though, these arguments never appear in the play function.

eloiqs commented 1 year ago

these arguments never appear in the play function

Landed here about this issue. It looks like from the typings we receive args in play, but when logging it in a jest test using composeStories it's always undefined no matter where the args are defined. The same console log in a live storybook yields the proper args. Workaround seems to be declaring the args in a constant or StoryName.args if only defined on the story.

shilman commented 1 year ago

@eloiqs It looks like you are expected to provide your own context to the play function when you're using composeStories. Example here:

https://github.com/storybookjs/storybook/blob/0bc5afc3e3dd9d4c62235ae8f2eceb41b0d55681/code/renderers/react/src/__test__/composeStories.test.tsx#L84-L93

What does your test look like?

eloiqs commented 1 year ago

@shilman My test looks something like

import { composeStories, composeStory, StoryFn } from '@storybook/react';
import projectAnnotations from '../../../.storybook/preview';
import * as stories from './CopyToClipboard.stories';
import { render } from '@testing-library/react';
import { act } from 'react-dom/test-utils';
const componentAnnotations = stories.default;

const Default1 = composeStory(stories.Default, componentAnnotations);

const { Default: Default2 } = composeStories(
  stories,
  projectAnnotations,
);

it('should display stuff 1', async () => {
  const { container } = render(<Default1 />);

  await act(async () => {
    await Default1.play({
      canvasElement: container,
    });
  });
});

it('should display stuff 2', async () => {
  const { container } = render(<Default2 />);

  await act(async () => {
    await Default2.play({
      canvasElement: container,
    });
  });
});

And the story under test looks like

import { Box } from '@mui/material';
import { expect } from '@storybook/jest';
import type { Meta, StoryObj } from '@storybook/react';
import { userEvent, within } from '@storybook/testing-library';
import { SomeComponent } from './SomeComponent';

export default {
  component: SomeComponent,
  args: {
    text: 'some text',
    label: 'some label',
  },
  decorators: [
    (Story, context) => (
      <Box
        sx={{
          backgroundColor: 'grey.800',
          color: 'white',
          padding: 2,
          width: 'fit-content',
        }}
      >
        {context.args.text}
        <Story />
      </Box>
    ),
  ],
} as Meta<typeof SomeComponent>;

type Story = StoryObj<typeof SomeComponent>;

export const Default: Story = {
  async play({ canvasElement, args }) {
    const canvas = within(canvasElement);
    const { label } = args;
    const stuff = await canvas.findByLabelText(`stuff`);

    await expect(canvasElement.querySelector('.some-stuff')).toBeInTheDocument();

    await userEvent.click(stuff);
  },
};

Even though I wasn't aware of the second arguments, passing componentAnnotations / projectAnnotations to composeStory / composeStories doesn't seem to change anything about the undefined args in play (ie const { label } = args throws "cannot read property label of undefined" when running the test).

yannbf commented 7 months ago

Hey everyone! Thanks a lot for elaborating all the issues. They have all been fixed and will be released in Storybook 8.1 (shortly after Storybook 8.0 is released). Thank you so much for your patience!! 🙏

storybookjs / storybook