Merge pull request #26765 from storybookjs/jeppe/rerender-queue

Core: Only render once when changing args while loading/rendering

code/e2e-tests/preview-web.spec.ts → code/e2e-tests/preview-api.spec.ts

@@ -5,7 +5,9 @@ import { SbPage } from './util';
 const storybookUrl = process.env.STORYBOOK_URL || 'http://localhost:8001';
 const templateName = process.env.STORYBOOK_TEMPLATE_NAME || '';
 
-test.describe('preview-web', () => {
+const wait = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms));
+
+test.describe('preview-api', () => {
   test.beforeEach(async ({ page }) => {
     await page.goto(storybookUrl);
 
@@ -50,4 +52,40 @@ test.describe('preview-web', () => {
     await sbPage.previewRoot().getByRole('button').getByText('Submit').first().press('Alt+s');
     await expect(sbPage.page.locator('.sidebar-container')).not.toBeVisible();
   });
+
+  // if rerenders were interleaved the button would have rendered "Error: Interleaved loaders. Changed arg"
+  test('should only render once at a time when rapidly changing args', async ({ page }) => {
+    const sbPage = new SbPage(page);
+    await sbPage.navigateToStory('lib/preview-api/rendering', 'slow-loader');
+
+    const root = sbPage.previewRoot();
+
+    const labelControl = await sbPage.page.locator('#control-label');
+
+    await expect(root.getByText('Loaded. Click me')).toBeVisible();
+    await expect(labelControl).toBeVisible();
+
+    await labelControl.fill('');
+    await labelControl.type('Changed arg', { delay: 50 });
+    await labelControl.blur();
+
+    await expect(root.getByText('Loaded. Changed arg')).toBeVisible();
+  });
+
+  test('should reload plage when remounting while loading', async ({ page }) => {
+    const sbPage = new SbPage(page);
+    await sbPage.navigateToStory('lib/preview-api/rendering', 'slow-loader');
+
+    const root = sbPage.previewRoot();
+
+    await expect(root.getByText('Loaded. Click me')).toBeVisible();
+
+    await sbPage.page.getByRole('button', { name: 'Remount component' }).click();
+    await wait(200);
+    await sbPage.page.getByRole('button', { name: 'Remount component' }).click();
+
+    // the loading spinner indicates the iframe is being fully reloaded
+    await expect(sbPage.previewIframe().locator('.sb-preparing-story > .sb-loader')).toBeVisible();
+    await expect(root.getByText('Loaded. Click me')).toBeVisible();
+  });
 });

code/lib/preview-api/src/modules/preview-web/PreviewWeb.test.ts

@@ -907,72 +907,82 @@ describe('PreviewWeb', () => {
     });
 
     describe('while story is still rendering', () => {
-      it('runs loaders again', async () => {
+      it('runs loaders again after renderToCanvas is done', async () => {
+        // Arrange - set up a gate to control when the loaders run
         const [loadersRanGate, openLoadersRanGate] = createGate();
         const [blockLoadersGate, openBlockLoadersGate] = createGate();
 
         document.location.search = '?id=component-one--a';
-        componentOneExports.default.loaders[0].mockImplementationOnce(async () => {
+        componentOneExports.default.loaders[0].mockImplementationOnce(async (input) => {
           openLoadersRanGate();
           return blockLoadersGate;
         });
 
+        // Act - render the first time
         await new PreviewWeb(importFn, getProjectAnnotations).ready();
         await loadersRanGate;
 
+        // Assert - loader to be called the first time
+        expect(componentOneExports.default.loaders[0]).toHaveBeenCalledOnce();
         expect(componentOneExports.default.loaders[0]).toHaveBeenCalledWith(
           expect.objectContaining({
             args: { foo: 'a', one: 'mapped-1' },
           })
         );
 
-        componentOneExports.default.loaders[0].mockClear();
+        // Act - update the args (while loader is still running)
         emitter.emit(UPDATE_STORY_ARGS, {
           storyId: 'component-one--a',
           updatedArgs: { new: 'arg' },
         });
-        await waitForRender();
 
-        expect(componentOneExports.default.loaders[0]).toHaveBeenCalledWith(
-          expect.objectContaining({
-            args: { foo: 'a', new: 'arg', one: 'mapped-1' },
-          })
-        );
+        // Arrange - open the gate to let the loader finish and wait for render
+        openBlockLoadersGate({ l: 8 });
+        await waitForRender();
 
-        // Story gets rendered with updated args
-        expect(projectAnnotations.renderToCanvas).toHaveBeenCalledTimes(1);
+        // Assert - renderToCanvas to be called the first time with initial args
+        expect(projectAnnotations.renderToCanvas).toHaveBeenCalledOnce();
         expect(projectAnnotations.renderToCanvas).toHaveBeenCalledWith(
           expect.objectContaining({
-            forceRemount: true, // Wasn't yet rendered so we need to force remount
+            forceRemount: true,
             storyContext: expect.objectContaining({
-              loaded: { l: 7 }, // This is the value returned by the *second* loader call
+              loaded: { l: 8 }, // This is the value returned by the *first* loader call
               args: { foo: 'a', new: 'arg', one: 'mapped-1' },
             }),
           }),
           'story-element'
         );
+        // Assert - loaders are not run again yet
+        expect(componentOneExports.default.loaders[0]).toHaveBeenCalledOnce();
 
-        // Now let the first loader call resolve
+        // Arrange - wait for loading and rendering to finish a second time
         mockChannel.emit.mockClear();
-        projectAnnotations.renderToCanvas.mockClear();
-        openBlockLoadersGate({ l: 8 });
         await waitForRender();
+        // Assert - loader is called a second time with updated args
+        await vi.waitFor(() => {
+          expect(projectAnnotations.renderToCanvas).toHaveBeenCalledTimes(2);
+          expect(componentOneExports.default.loaders[0]).toHaveBeenCalledWith(
+            expect.objectContaining({
+              args: { foo: 'a', new: 'arg', one: 'mapped-1' },
+            })
+          );
+        });
 
-        // Now the first call comes through, but picks up the new args
-        // Note this isn't a particularly realistic case (the second loader being quicker than the first)
-        expect(projectAnnotations.renderToCanvas).toHaveBeenCalledTimes(1);
+        // Assert - renderToCanvas is called a second time with updated args
+        expect(projectAnnotations.renderToCanvas).toHaveBeenCalledTimes(2);
         expect(projectAnnotations.renderToCanvas).toHaveBeenCalledWith(
           expect.objectContaining({
+            forceRemount: false,
             storyContext: expect.objectContaining({
-              loaded: { l: 8 },
+              loaded: { l: 7 }, // This is the value returned by the *second* loader call
               args: { foo: 'a', new: 'arg', one: 'mapped-1' },
             }),
           }),
           'story-element'
         );
       });
 
-      it('renders a second time if renderToCanvas is running', async () => {
+      it('renders a second time after the already running renderToCanvas is done', async () => {
         const [gate, openGate] = createGate();
 
         document.location.search = '?id=component-one--a';
@@ -986,11 +996,9 @@ describe('PreviewWeb', () => {
           updatedArgs: { new: 'arg' },
         });
 
-        // Now let the renderToCanvas call resolve
+        // Now let the first renderToCanvas call resolve
         openGate();
-        await waitForRender();
-
-        expect(projectAnnotations.renderToCanvas).toHaveBeenCalledTimes(2);
+        expect(projectAnnotations.renderToCanvas).toHaveBeenCalledTimes(1);
         expect(projectAnnotations.renderToCanvas).toHaveBeenCalledWith(
           expect.objectContaining({
             forceRemount: true,
@@ -1001,39 +1009,14 @@ describe('PreviewWeb', () => {
           }),
           'story-element'
         );
-        expect(projectAnnotations.renderToCanvas).toHaveBeenCalledWith(
-          expect.objectContaining({
-            forceRemount: false,
-            storyContext: expect.objectContaining({
-              loaded: { l: 7 },
-              args: { foo: 'a', new: 'arg', one: 'mapped-1' },
-            }),
-          }),
-          'story-element'
-        );
-      });
 
-      it('works if it is called directly from inside non async renderToCanvas', async () => {
-        document.location.search = '?id=component-one--a';
-        projectAnnotations.renderToCanvas.mockImplementation(() => {
-          emitter.emit(UPDATE_STORY_ARGS, {
-            storyId: 'component-one--a',
-            updatedArgs: { new: 'arg' },
-          });
-        });
-        await createAndRenderPreview();
+        // Wait for the second render to finish
+        mockChannel.emit.mockClear();
+        await waitForRender();
+        await waitForRenderPhase('rendering');
 
+        // Expect the second render to have the updated args
         expect(projectAnnotations.renderToCanvas).toHaveBeenCalledTimes(2);
-        expect(projectAnnotations.renderToCanvas).toHaveBeenCalledWith(
-          expect.objectContaining({
-            forceRemount: true,
-            storyContext: expect.objectContaining({
-              loaded: { l: 7 },
-              args: { foo: 'a', one: 'mapped-1' },
-            }),
-          }),
-          'story-element'
-        );
         expect(projectAnnotations.renderToCanvas).toHaveBeenCalledWith(
           expect.objectContaining({
             forceRemount: false,

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

@@ -81,6 +81,58 @@ describe('StoryRender', () => {
     expect(story.playFunction).not.toHaveBeenCalled();
   });
 
+  it('only rerenders once when triggered multiple times while pending', async () => {
+    // Arrange - setup StoryRender and async gate blocking applyLoaders
+    const [loaderGate, openLoaderGate] = createGate();
+    const story = {
+      id: 'id',
+      title: 'title',
+      name: 'name',
+      tags: [],
+      applyLoaders: vi.fn(() => loaderGate),
+      unboundStoryFn: vi.fn(),
+      playFunction: vi.fn(),
+      prepareContext: vi.fn(),
+    };
+    const store = { getStoryContext: () => ({}), cleanupStory: vi.fn() };
+    const renderToScreen = vi.fn();
+    const render = new StoryRender(
+      new Channel({}),
+      store as any,
+      renderToScreen,
+      {} as any,
+      entry.id,
+      'story',
+      { autoplay: true },
+      story as any
+    );
+    // Arrange - render (blocked by loaders)
+    render.renderToElement({} as any);
+    expect(story.applyLoaders).toHaveBeenCalledOnce();
+    expect(render.phase).toBe('loading');
+
+    // Act - rerender 3x
+    render.rerender();
+    render.rerender();
+    render.rerender();
+
+    // Assert - still loading, not yet rendered
+    expect(story.applyLoaders).toHaveBeenCalledOnce();
+    expect(render.phase).toBe('loading');
+    expect(renderToScreen).not.toHaveBeenCalled();
+
+    // Act - finish loading
+    openLoaderGate();
+
+    // Assert - loaded and rendered twice, played once
+    await vi.waitFor(async () => {
+      console.log(render.phase);
+      expect(story.applyLoaders).toHaveBeenCalledTimes(2);
+      expect(renderToScreen).toHaveBeenCalledTimes(2);
+      expect(story.playFunction).toHaveBeenCalledOnce();
+    });
+  });
+
   describe('teardown', () => {
     it('throws PREPARE_ABORTED if torndown during prepare', async () => {
       const [importGate, openImportGate] = createGate();

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

@@ -56,6 +56,8 @@ export class StoryRender<TRenderer extends Renderer> implements Render<TRenderer
 
   private notYetRendered = true;
 
+  private rerenderEnqueued = false;
+
   public disableKeyListeners = false;
 
   private teardownRender: TeardownRenderToCanvas = () => {};
@@ -268,10 +270,27 @@ export class StoryRender<TRenderer extends Renderer> implements Render<TRenderer
       this.phase = 'errored';
       this.callbacks.showException(err as Error);
     }
+
+    // If a rerender was enqueued during the render, clear the queue and render again
+    if (this.rerenderEnqueued) {
+      this.rerenderEnqueued = false;
+      this.render();
+    }
   }
 
+  /**
+   * Rerender the story.
+   * If the story is currently pending (loading/rendering), the rerender will be enqueued,
+   * and will be executed after the current render is completed.
+   * Rerendering while playing will not be enqueued, and will be executed immediately, to support
+   * rendering args changes while playing.
+   */
   async rerender() {
-    return this.render();
+    if (this.isPending() && this.phase !== 'playing') {
+      this.rerenderEnqueued = true;
+    } else {
+      return this.render();
+    }
   }
 
   async remount() {

code/lib/preview-api/template/stories/rendering.stories.ts

@@ -66,3 +66,31 @@ export const ChangeArgs = {
     await expect(button).toHaveFocus();
   },
 };
+
+let loadedLabel = 'Initial';
+
+/**
+ * This story demonstrates what happens when rendering (loaders) have side effects, and can possibly interleave with each other
+ * Triggering multiple force remounts quickly should only result in a single remount in the end
+ * and the label should be 'Loaded. Click Me' at the end. If loaders are interleaving it would result in a label of 'Error: Interleaved loaders. Click Me'
+ * Similarly, changing args rapidly should only cause one rerender at a time, producing the same result.
+ */
+export const SlowLoader = {
+  loaders: [
+    async () => {
+      loadedLabel = 'Loading...';
+      await new Promise((resolve) => setTimeout(resolve, 1000));
+      loadedLabel = loadedLabel === 'Loading...' ? 'Loaded.' : 'Error: Interleaved loaders.';
+      return { label: loadedLabel };
+    },
+  ],
+  decorators: [
+    (storyFn: any, context: any) =>
+      storyFn({
+        args: {
+          ...context.args,
+          label: `${context.loaded.label} ${context.args.label}`,
+        },
+      }),
+  ],
+};