storybookjs / storybook

Storybook is the industry standard workshop for building, documenting, and testing UI components in isolation
https://storybook.js.org
MIT License
84.77k stars 9.34k forks source link

[Feature Request]: Create pre play functionality #21252

Open donaldpipowitch opened 1 year ago

donaldpipowitch commented 1 year ago

Is your feature request related to a problem? Please describe

I'd like to pass data to my play functions so I can run expect's on it. For example all my Stories use independent Axios instances I could get via a custom useAxiosInstance hook or I'd like to check location from React Routers useLocation.

Describe the solution you'd like

It would be amazing to use hooks in play functions. As I don't think this will be possible/useful/expected I could imagine something similar could be done with a "pre play functionality" where React hooks are allowed and where the returned values are passed to play.

// meta.decorators uses `MemoryRouter` from react-router-dom

export const Example: Story = {
  prePlay: () => {
    const location = useLocation();
    return { location };
  },
  play: async ({ canvasElement, setup }) => { // setup would be `null` if no `prePlay` is used
    const canvas = within(canvasElement);

    await expect(
      await canvas.findByText('Lorem')
    ).toBeInTheDocument();
    expect(setup.location.pathname).toBe('/example/users');
  },
};

Describe alternatives you've considered

Allow hooks in play directly or somehow access decorators inside play where I could maybe grab values from.

I'm just wondering... what if MemoryRouter is not defined in meta.decorators but as part of meta.component? Maybe the control should be "inversed" and my component could pass data to play. Something like this:

// in my component
const location = useLocation();
usePlaySetup({ location }); // would be a noop in non-storybook build

The only problem would be: play can't really know if usePlaySetup will be called. This would have to be synchronous or that would need to be a "flag" that indicates play should only be run when usePlaySetup was called. Some "deferred" play option...

Are you able to assist to bring the feature to reality?

yes, I can

Additional context

No response

terrymun commented 1 year ago

It sounds like something that can possibly be achieved by mocking imports? Assuming that you are importing useLocation from a dependency, you can mock the import using this guide here.

shilman commented 1 year ago

We're planning to revisit both of these areas (adding more test hooks + import mocking) in 7.x, so this is a timely request. cc @yannbf @kasperpeulen @tmeasday

donaldpipowitch commented 1 year ago

It sounds like something that can possibly be achieved by mocking imports?

I'd assume this is not a solution, because the problem (or one of the problems) is that I can't use React hooks inside the play function - no matter if it is a mocked hook or not. (And I'd like to use the real hook.)

We're planning to revisit both of these areas (adding more test hooks + import mocking) in 7.x

Sounds great, thanks! Maybe to give this a different spin: Testing Library recommends to use a custom render function. This is basically how we used it as well (e.g. to pass individual Axios instances per Story that are spied individually as well).

Maybe the API would look more like this?

export const Example: Story = {
  // customRender could be globally shared across all stories
  customRender: (story) => {
    const axiosInstance = createAxiosInstance();
    // add some Jest spies here on axiosInstance
    // maybe storybook exposes a "renderStory" function that is already a custom render function, but can be further wrapped
    const result = renderStory(<AxiosInstance value={axiosInstance}>{story}</AxiosInstance>);
    return {
     canvas: within(canvasElement), // maybe we can also safe us some time to do this in every `play` function
     axiosInstance,
     ...result
   }
  },
  play: async ({ canvas, axiosInstance, ...rest }) => {
    await expect(
      await canvas.findByText('Lorem')
    ).toBeInTheDocument();
    // run expects here on axiosInstance spies
  },
};
kasperpeulen commented 1 year ago

@donaldpipowitch I have had similar problems with testing in Storybook. I don't think we have anything recommended, but there is this hack around it.

const meta = {
  title: 'Example/Page',
  component: Page,
  decorators: [
    (Story, context) => {
      const location = useLocation();
      context.args.onLocationChanged(location);
      return <Story />;
    },
    (Story) => {
      return (
        <MemoryRouter initialEntries={['/page1']}>
          <Routes>
            <Route path={'page1'} element={<Story />}></Route>
          </Routes>
        </MemoryRouter>
      );
    },
  ],
  argTypes: {
    onLocationChanged: { action: true },
  },
} satisfies Meta<ComponentProps<typeof Page> & { onLocationChanged(location: Location): void }>;

export default meta;
type Story = StoryObj<typeof meta>;

export const LoggedIn: Story = {
  play: async (context) => {
    const canvas = within(context.canvasElement);

    const args = context.args as jest.Mocked<typeof context.args>;

    const [location] = args.onLocationChanged.mock.lastCall!;
    console.log(location);

    const link = await canvas.findByRole('link', { name: /some link/i });
    await userEvent.click(link);
    await waitFor(() => args.onLocationChanged.mock.calls.length > 1);

    const [location2] = args.onLocationChanged.mock.lastCall!;
    console.log(location2);
  },
};

I thought we could also inject context and custom args in decorators, but for some reason I can not get it to work. @tmeasday @shilman Do you know more about this? For example:

  decorators: [
    (Story, context) => {
      const location = useLocation();
      return <Story location={location} args={{location}} />;
    },
donaldpipowitch commented 1 year ago

Thank you @kasperpeulen !

tmeasday commented 1 year ago

@kasperpeulen I would have expected what you did (or something like it) to work. There are certain context fields you aren't allowed to update from decorators, but location and args.location aren't amongst them: https://github.com/storybookjs/storybook/blob/7aecf57f9e2fb9d89efd14814ae7412052a4eae9/code/lib/preview-api/src/modules/store/decorators.ts#L23-L43

kasperpeulen commented 1 year ago

They are passed down to the render function, but not to the play function it seems:

image image

Is this a bug @tmeasday @shilman ?

tmeasday commented 1 year ago

I think yes @kasperpeulen. Thinking about it, it might be tricky to fix this however. We could look at it together sometime.

sjwilczynski commented 1 year ago

@tmeasday, @kasperpeulen did you do something about discrepancy of context in render and play function? Having that working would enable me to create a decorator which exposes functionality during execution of play function

tmeasday commented 1 year ago

Not yet @sjwilczynski! We talked around it but I think this issue escaped our attention. @kasperpeulen let's find some time to go over it when you are back!

yannbf commented 1 year ago

Thinking about it, it might be tricky to fix this however. We could look at it together sometime.

I don't even know if it's possible @tmeasday, given that we would need to somehow "prerender" the story in order for decorators to run, and only then get the "final" context into the play function. This is where we define the play function: https://github.com/storybookjs/storybook/blob/next/code/lib/preview-api/src/modules/store/csf/prepareStory.ts#L92

tmeasday commented 1 year ago

@yannbf it is a bit tricky. Right above that code we wrap the undecoratedStoryFn in the decorators.

I think what we want is the context that was passed into that undecoratedStoryFn the previous time before the play function is called.

One option would be to make the PreparedStory stateful so it "remembers" that context (feels like a code smell but would work).

Another is to make the preparedStory.boundStoryFn() return the context, or even a boundPlayFunction (bound with that context). However that'd be a little difficult because the renderer would need to return that to the StoryRender (which calls the play function). So we'd need to alter every renderer to do that, which would technically be a breaking change.

Also it might make the portable stories use case more difficult as they can no longer just call composedStory.play(), they need to do composedStory.play(contextFromStoryFn). Which makes the stateful thing seem more interesting :)

sjwilczynski commented 1 year ago

Interestingly I just realized that even tough assigning property to a context doesn't make it available in play function, one can still edit parameters inside decorator as long as they are not reassigned but just new property is added:


//decorator code
makeDecorator({
    name: "withNovaEnvironment",
    parameterName: "novaEnvironment",
    wrapper: (getStory, context, settings) => {
      const environment = createNovaEnvironment();

    // code omitted for brevity 

      context.parameters.environment = environment;
      return (
        <NovaMockEnvironmentProvider environment={environment}>
          {getStory(context)}
        </NovaMockEnvironmentProvider>
      );
    },
  });

// inside play function

play: async (context) => {

    // this is properly defined
    console.log(context.parameters.environment);
  },

@yannbf, @tmeasday do you think we could safely depend on this behavior?

tmeasday commented 1 year ago

Oh, I don't think I would count on that sticking around forever, we could definitely break it without considering it a breaking change. Having said that, it's probably not something that is likely to stop working any time soon.

nandin-borjigin commented 1 year ago

This is where the context is shallow-copied before being passed into decorator, while the play function is receiving the original context object. Any root level modification to the context object in decorator is applied on this shallow copy, which is not accessible in play function.

I wonder why the context was shallow-copied in the first place. Is it because the very first decorator's context object is a React prop thus it's not extensible? If that is the case, can we actually shallow copy at the beginning and stick to the copied context object in both decorators and play function?

tmeasday commented 1 year ago

@nandin-borjigin we'll have to think about that. Generally speaking the SB codebase assumes objects are immutable and thus generally cloned rather than having things added to them as they are passed around.

yannbf commented 2 weeks ago

Hey everyone! It's been some time, but I'm here with some news. In Storybook 8, we introduced a new way of writing the play function (check the RFC) which should give you full flexibility on what you want to do.

export const Example: Story = {
  play: async ({ canvas, mount }) => {
    // Do anything before mounting
    const someAsyncData = await fetchData()
    // Mount your story however you want
    await mount(<YourComponent data={someAsyncData} />)

    // write your test
    const canvas = within(canvasElement);
    await expect(await canvas.findByText('Lorem')).toBeInTheDocument();
    expect(setup.location.pathname).toBe('/example/users');
  },
};

The other thing is that loaders can now mutate the story context, which can be useful for some use cases mentioned here.

export const Example: Story = {
  loaders: async (context) => { // loaders are executed before the component renders
    context.location = useLocation();
  },
  play: async ({ canvas, location }) => { // location is now available in the context of play function (as well as other places like decorators)
    await expect(
      await canvas.findByText('Lorem')
    ).toBeInTheDocument();
    await expect(setup.location.pathname).toBe('/example/users');
  },
};