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

[Feature Request]: Create pre play functionality #21252

Open
donaldpipowitch opened this issue Feb 24, 2023 · 17 comments
Open

[Feature Request]: Create pre play functionality #21252

donaldpipowitch opened this issue Feb 24, 2023 · 17 comments

Comments

@donaldpipowitch
Copy link
Contributor

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
Copy link

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 shilman added csf and removed needs triage labels Feb 27, 2023
@shilman
Copy link
Member

shilman commented Feb 27, 2023

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
Copy link
Contributor Author

donaldpipowitch commented Feb 27, 2023

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
Copy link
Contributor

@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
Copy link
Contributor Author

Thank you @kasperpeulen !

@tmeasday
Copy link
Member

@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:

/**
* Currently StoryContextUpdates are allowed to have any key in the type.
* However, you cannot overwrite any of the build-it "static" keys.
*
* @param inputContextUpdate StoryContextUpdate
* @returns StoryContextUpdate
*/
export function sanitizeStoryContextUpdate({
componentId,
title,
kind,
id,
name,
story,
parameters,
initialArgs,
argTypes,
...update
}: StoryContextUpdate = {}): StoryContextUpdate {
return update;
}

@kasperpeulen
Copy link
Contributor

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
Copy link
Member

tmeasday commented Feb 28, 2023

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

@sjwilczynski
Copy link
Contributor

@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
Copy link
Member

tmeasday commented May 9, 2023

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
Copy link
Member

yannbf commented May 10, 2023

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
Copy link
Member

tmeasday commented May 11, 2023

@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
Copy link
Contributor

sjwilczynski commented May 16, 2023

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
Copy link
Member

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
Copy link

nandin-borjigin commented May 17, 2023

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
Copy link
Member

@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
Copy link
Member

yannbf commented Nov 8, 2024

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');
  },
};

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants