[RFC] beforeEach as a story hook for imperative code

[kasperpeulen]

Status: implemented

At the moment, Storybook is designed to work well when you can pass state in a fully declarative way. For example, pass data to a component or context provider like below:

const Story = {
  args: {
    some: 'value',
  },
  render: (args) => <Dialog {...args} />,
  decorators: [
    (Story) => <ThemeProvider value="dark"><Story/></ThemeProvider>
  ],
}

However, there might be state that need to be set in a more imperative way.
In general, any singleton data that lives outside of the component tree.

A good example is setting mock data to the spies that you can create with @storybook/test. What is the natural place to set a mock implementation?

spyOn(API, "getAllTodos").mockResolvedValue([
  { description: "Write a beforeRender RFC" },
]);

Or an imperative call to change the global Date instance:

import MockDate from 'mockdate';

MockDate.set('2000-11-22');

For those kind of use cases we are introducing beforeEach as a new CSF concept:

const Story = {
  beforeEach() {
    spyOn(API, "getAllTodos").mockResolvedValue([
      { description: "Write a beforeRender RFC" },
    ]);
    spyOn(console, 'log');
    MockDate.set('2000-11-22');
  }
}

When you attach beforeEach it to the default export, it will run beforeEach story. Before the component is mounted or the play function.

export default {
  component: Calendar,
  beforeRender() {
    // will run before each story, and also before the beforeRender on the story level
    MockDate.set('2000-11-22');
  }
}

Similar as beforeEach in vitest, it can accept an async function, and you can return a cleanup function as well:

const Story = {
  async beforeRender() {
    MockDate.set('2000-11-22');
    return MockDate.reset;
  }
}

However, we recommend in general, to cleaning up any hanging state in a preview beforeEach function:

import { restoreAllMocks } from '@storybook/test';

const preview = {
  beforeEach() {
    restoreAllMocks(); // we do this automatically already
    MockData.reset();
    msw.resetAllHandlers(); // happens automatically if you install the msw addon
  },
}

The preview function will run before the beforeEach defined on the default export, and both will run before the beforeEeach defined on the story.

We want users to still be able to work with Storybook in fully declarative way, even when dealing with state that is fundamentally not declarative, as the examples above. For that we want to introduce extend the concept of story args.

At this moment all args of the story are automatically applied to the component. We would like to introduce “$-prefixed-args” as a way to make sure the arg is not automatically applied to the component. Those non-component args, can then be used to work mock data in a fully declerative way (and also controllable in the args panel):

import { restoreAllMocks } from '@storybook/test';
import MockDate from 'mockdate';

const preview = {
  beforeRender({ args }) {
    if (args.$date) {
      MockData.set(args.$date)
    } else {
      MockData.reset();
    }
  },
}

// in your story
export default { component: Calendar };

const CalendarViewOnSunday = {
  args: {
    $date: new Date(...), // not applied to the component,
    events: [...] // applied to Calendar component,
  }
}

[JReinhold]

How to handle aborting the rendering pipeline?

Related: #26723

The proposal for the new pipeline is:

1. apply loaders
2. beforeRender
3. decorators
4. render
5. renderToCanvas
6. play
7. ... nothing
8. run any cleanup functions as returned from beforeRender

The problem

Any of these steps could take a long time to finish, and the user could navigate to another story during or between any of these steps. If not handled properly, this could cause race conditions with multiple stories loading/playing/anything simultaneously, while depending on the same global variable. This will especially become a problem with our module-mock proposal which relies on global mocks being configured and restored in each story. Essentially any pattern with side-effects will have issues with this.

Here is a demonstration of race conditions happening when loading multiple stories: https://stackblitz.com/~/edit/github-hhu8hi

It's just using a global variable, but imagine it was a mock that got changed with mockReturnValue, resetMock, etc.

Proposed solutions

1. Reload the preview iframe

Ideally we would have a mechanism that:

1. aborts a running phase (loading, rendering, playing, etc.)
2. runs cleanup functions
3. starts the next story pipeline

But we can't abort a running async function mid way, so we can't stop it from producing side effects after navigation. The only way to achieve this would be to trigger a full reload of the preview if we detect that we are in a running phase that can't be aborted. We already do this when we detect a running play-function?

I feared that reloading the iframe would be slow, so I ran some numbers by uploading a monorepo-sandbox to Chromatic (350 MBit/s download, Europe).
Navigating from the "Slow Play" story to the "Primary" story while still playing took:

• Online Build, Cache Disabled: 1.5s - 4s to complete, 1 MB transferred.
• Online Build, Cache Enabled: 500ms - 600ms, 25 kB transferred.
• Local Dev, Cache Disabled: 500ms - 600ms, 1.7 MB transferred.
• Local Dev, Cache Disabled: 300ms - 400ms, 388 kB transferred.

2. Wait for phase to finish, move cleanup to beginning of pipeline

We could also wait for the current phase to finish completely before navigating to the new story, ensuring that it doesn't produce side-effects after navigation. This could make the user experience slower and we might need to consider some UI feedback in this "waiting" state.

We would still need to run cleanup functions, but if we moved those to the beginning of the rendering pipeline, we would always ensure they run. This is already what we do today when we reset mocks as the first loader.

[JReinhold]

If my benchmarks are correct I think it's okay to do the full reload of the iframe when the user navigates before the full pipeline is done - ie. expanding the current play-detection to cover the whole pipeline.

My reasoning being:

1. I assume it's the lowest effort to implement, compared to being smart about aborting the pipeline and waiting for it
2. Navigating to another story while the first one is still in progress should be considered an edge case and the experience doesn't have to be ultra-fast, it just have to not cause a confusing state.
3. Really only the Online+Disabled Cache numbers are worrying, and that seems like a combination of edgy-edge-cases that I think we don't have to optimize for.

Is there anything I'm missing here, some behind-the-scenes considerations I haven't accounted for?

An interesting related question, should we also reload when users force-remount before the pipeline is done? Try opening the "Slow Play" story, open the Interactions panel and spam the force re-mount button. You'll see the same assertion being logged multiple times, which looks like a similar race condition to the one described above.

[tmeasday]

@JReinhold -- FYI @ghengeveld and I dealt with this issue (confined to play functions, mainly) already as part of the interaction debugger project. We struggled with it a bit, but dealt with in mainly in two ways:

1. Ensured that our "instrumented / intercepted" testing library code (which is all promise wrapped as it can be paused by the debugger) is sensitive to the StoryRender's abortController. That is, all the promises will self-cancel (where possible) when the abort is triggered. I'm not sure if there are cases where we can't do that, like if it's mid way through a userEvent.type(...) with a delay.

2. Added this slightly hacky teardown block to StoryRender:

   storybook/code/lib/preview-api/src/modules/preview-web/render/StoryRender.ts

   Lines 287 to 311 in 89e8456

   	async teardown() {
   	this.torndown = true;
   	this.cancelRender();
   	// If the story has loaded, we need to cleanup
   	if (this.story) this.store.cleanupStory(this.story);
   	// Check if we're done rendering/playing. If not, we may have to reload the page.
   	// Wait several ticks that may be needed to handle the abort, then try again.
   	// Note that there's a max of 5 nested timeouts before they're no longer "instant".
   	for (let i = 0; i < 3; i += 1) {
   	if (!this.isPending()) {
   	await this.teardownRender();
   	return;
   	}
   	await new Promise((resolve) => setTimeout(resolve, 0));
   	}
   	// If we still haven't completed, reload the page (iframe) to ensure we have a clean slate
   	// for the next render. Since the reload can take a brief moment to happen, we want to stop
   	// further rendering by awaiting a never-resolving promise (which is destroyed on reload).
   	window.location.reload();
   	await new Promise(() => {});
   	}

• Basically we abort the controller, then wait for 3 setTimeout(0)'s (why 3? I'm not really sure, TBH).
• If the play or renderToCanvas function hasn't finished in this time, we then do the reload.

So I guess there's no reason to not continue with this approach? Perhaps we can try and make any other async libraries that we provide sensitive to the current abort controller (if possible), and ensure isPending() is also true during loaders and beforeRender.

FWIW decorators and render are sync so don't have this issue, but you left renderToCanvas (which is async) out of your stack above.

[tmeasday]

I don't remember this for sure, but I suspect the reason we left loaders out of the initial implementation is that we considered them "side effect free" and the rendering pipeline will stop immediately after the loaders finish (if aborted), so we figured we could just change story immediately if a loader was mid-way through.

This may have been a been a bad assumption.

[JReinhold]

So I guess there's no reason to not continue with this approach? Perhaps we can try and make any other async libraries that we provide sensitive to the current abort controller (if possible), and ensure isPending() is also true during loaders and beforeRender.

Yes that's also the solution I'm leaning towards. I'll try that out, expanding the reload-functionality to cover loaders and beforeRender.