Merge pull request #18432 from storybookjs/tom/sb-347-sequence-import…

…ing-stories-in-parallel

addons/storyshots/storyshots-core/package.json

@@ -43,6 +43,7 @@
     "@storybook/client-api": "7.0.0-alpha.5",
     "@storybook/core-client": "7.0.0-alpha.5",
     "@storybook/core-common": "7.0.0-alpha.5",
+    "@storybook/core-webpack": "7.0.0-alpha.5",
     "@storybook/csf": "0.0.2--canary.4566f4d.1",
     "@types/glob": "^7.1.3",
     "@types/jest": "^26.0.16",

addons/storyshots/storyshots-core/src/frameworks/configure.ts

@@ -1,7 +1,8 @@
 import fs from 'fs';
 import path from 'path';
 import type { NormalizedStoriesSpecifier, StoriesEntry } from '@storybook/core-common';
-import { toRequireContext, normalizeStoriesEntry } from '@storybook/core-common';
+import { toRequireContext } from '@storybook/core-webpack';
+import { normalizeStoriesEntry } from '@storybook/core-common';
 import registerRequireContextHook from '@storybook/babel-plugin-require-context-hook/register';
 import global from 'global';
 import type {

lib/builder-webpack5/src/preview/iframe-webpack.config.ts

@@ -9,17 +9,16 @@ import TerserWebpackPlugin from 'terser-webpack-plugin';
 import VirtualModulePlugin from 'webpack-virtual-modules';
 import ForkTsCheckerWebpackPlugin from 'fork-ts-checker-webpack-plugin';
 
-import type { Options, CoreConfig } from '@storybook/core-common';
+import type { Options, CoreConfig, Webpack5BuilderConfig } from '@storybook/core-common';
 import {
-  toRequireContextString,
   stringifyProcessEnvs,
   handlebars,
   interpolate,
-  toImportFn,
   normalizeStories,
   readTemplate,
   loadPreviewOrConfigFile,
 } from '@storybook/core-common';
+import { toRequireContextString, toImportFn } from '@storybook/core-webpack';
 import type { TypescriptOptions } from '../types';
 import { createBabelLoader } from './babel-loader-preview';
 
@@ -103,7 +102,9 @@ export default async (
     const storiesFilename = 'storybook-stories.js';
     const storiesPath = path.resolve(path.join(workingDir, storiesFilename));
 
-    virtualModuleMapping[storiesPath] = toImportFn(stories);
+    const needPipelinedImport =
+      !!(coreOptions.builder as Webpack5BuilderConfig).options?.lazyCompilation && !isProd;
+    virtualModuleMapping[storiesPath] = toImportFn(stories, { needPipelinedImport });
     const configEntryPath = path.resolve(path.join(workingDir, 'storybook-config-entry.js'));
     virtualModuleMapping[configEntryPath] = handlebars(
       await readTemplate(

lib/core-common/src/index.ts

@@ -21,9 +21,8 @@ export * from './utils/cache';
 export * from './utils/template';
 export * from './utils/interpolate';
 export * from './utils/validate-configuration-files';
-export * from './utils/to-require-context';
+export * from './utils/glob-to-regexp';
 export * from './utils/normalize-stories';
-export * from './utils/to-importFn';
 export * from './utils/readTemplate';
 export * from './utils/findDistEsm';
 

lib/core-webpack/src/importPipeline.test.ts

@@ -0,0 +1,121 @@
+import { importPipeline } from './importPipeline';
+
+const createGate = (): [Promise<any | undefined>, (_?: any) => void] => {
+  let openGate = (_?: any) => {};
+  const gate = new Promise<any | undefined>((resolve) => {
+    openGate = resolve;
+  });
+  return [gate, openGate];
+};
+
+it('passes through to passed importFn on serial calls', async () => {
+  const pipeline = importPipeline();
+  const importFn = jest.fn();
+
+  importFn.mockResolvedValueOnce('r1');
+  expect(await pipeline(() => importFn('i1'))).toEqual('r1');
+  expect(importFn).toHaveBeenCalledTimes(1);
+  expect(importFn).toHaveBeenCalledWith('i1');
+
+  importFn.mockResolvedValueOnce('r2');
+  expect(await pipeline(() => importFn('i2'))).toEqual('r2');
+  expect(importFn).toHaveBeenCalledTimes(2);
+  expect(importFn).toHaveBeenCalledWith('i2');
+});
+
+it('blocks on parallel calls', async () => {
+  const pipeline = importPipeline();
+  const [firstGate, openFirstGate] = createGate();
+  const importFn = jest
+    .fn()
+    .mockImplementationOnce(() => firstGate)
+    .mockResolvedValueOnce('r2');
+
+  const firstPromise = pipeline(() => importFn('i1'));
+
+  // We need to await promise resolution to get the block setup
+  await new Promise((r) => r(null));
+
+  const secondPromise = pipeline(() => importFn('i2'));
+
+  expect(importFn).toHaveBeenCalledTimes(1);
+  expect(importFn).toHaveBeenCalledWith('i1');
+  openFirstGate('r1');
+  expect(await firstPromise).toEqual('r1');
+
+  // We need to await promise resolution to get past the block
+  await new Promise((r) => r(null));
+
+  expect(importFn).toHaveBeenCalledTimes(2);
+  expect(importFn).toHaveBeenCalledWith('i2');
+  expect(await secondPromise).toEqual('r2');
+});
+
+it('dispatches all queued calls on opening', async () => {
+  const pipeline = importPipeline();
+  const [firstGate, openFirstGate] = createGate();
+  const importFn = jest
+    .fn()
+    .mockImplementationOnce(() => firstGate)
+    .mockResolvedValueOnce('r2')
+    .mockResolvedValueOnce('r3');
+
+  const firstPromise = pipeline(() => importFn('i1'));
+
+  // We need to await promise resolution to get the block setup
+  await new Promise((r) => r(null));
+  const secondPromise = pipeline(() => importFn('i2'));
+
+  // We need to await promise resolution to get the block setup
+  await new Promise((r) => r(null));
+  const thirdPromise = pipeline(() => importFn('i3'));
+
+  expect(importFn).toHaveBeenCalledTimes(1);
+  expect(importFn).toHaveBeenCalledWith('i1');
+  openFirstGate('r1');
+  expect(await firstPromise).toEqual('r1');
+
+  // We need to await promise resolution to get past the block
+  await new Promise((r) => r(null));
+
+  expect(importFn).toHaveBeenCalledTimes(3);
+  expect(importFn).toHaveBeenCalledWith('i2');
+  expect(importFn).toHaveBeenCalledWith('i3');
+  expect(await secondPromise).toEqual('r2');
+  expect(await thirdPromise).toEqual('r3');
+});
+
+it('blocks sequentially on parallel calls', async () => {
+  const pipeline = importPipeline();
+  const [firstGate, openFirstGate] = createGate();
+  const [secondGate, openSecondGate] = createGate();
+  const importFn = jest
+    .fn()
+    .mockImplementationOnce(() => firstGate)
+    .mockImplementationOnce(() => secondGate)
+    .mockResolvedValueOnce('r3');
+
+  const firstPromise = pipeline(() => importFn('i1'));
+
+  // We need to await promise resolution to get the block setup
+  await new Promise((r) => r(null));
+  const secondPromise = pipeline(() => importFn('i2'));
+
+  expect(importFn).toHaveBeenCalledTimes(1);
+  expect(importFn).toHaveBeenCalledWith('i1');
+  openFirstGate('r1');
+  expect(await firstPromise).toEqual('r1');
+
+  // We need to await promise resolution to get past the block, and set up the new one
+  await new Promise((r) => r(null));
+  const thirdPromise = pipeline(() => importFn('i3'));
+
+  expect(importFn).toHaveBeenCalledTimes(2);
+  expect(importFn).toHaveBeenCalledWith('i2');
+  openSecondGate('r2');
+  expect(await secondPromise).toEqual('r2');
+
+  // We need to await promise resolution to get past the block
+  await new Promise((r) => r(null));
+  expect(await thirdPromise).toEqual('r3');
+});

lib/core-webpack/src/importPipeline.ts

@@ -0,0 +1,22 @@
+type ModuleExports = Record<string, any>;
+
+// If an import is in flight when another import arrives, block it until the first completes.
+// This is to avoid a situation where webpack kicks off a second compilation whilst the
+// first is still completing, cf: https://github.com/webpack/webpack/issues/15541#issuecomment-1143138832
+// Note the way this code works if N futher `import()`s occur while the first is in flight,
+// they will all be kicked off in the same tick and not block each other. This is by design,
+// Webpack can handle multiple invalidations simutaneously, just not in quick succession.
+
+export function importPipeline() {
+  let importGate: Promise<void> = Promise.resolve();
+
+  return async (importFn: () => Promise<ModuleExports>) => {
+    await importGate;
+
+    const moduleExportsPromise = importFn();
+    importGate = importGate.then(async () => {
+      await moduleExportsPromise;
+    });
+    return moduleExportsPromise;
+  };
+}

lib/core-webpack/src/index.ts

@@ -2,3 +2,5 @@ export * from './types';
 export * from './load-custom-webpack-config';
 export * from './check-webpack-version';
 export * from './merge-webpack-config';
+export * from './to-importFn';
+export * from './to-require-context';

lib/core-common/src/utils/__tests__/to-importFn.test.ts → lib/core-webpack/src/to-importFn.test.ts

@@ -1,5 +1,6 @@
-import { webpackIncludeRegexp } from '../to-importFn';
-import { normalizeStoriesEntry } from '../normalize-stories';
+import { normalizeStoriesEntry } from '@storybook/core-common';
+
+import { webpackIncludeRegexp } from './to-importFn';
 
 const testCases: [string, string[], string[]][] = [
   [

lib/core-common/src/utils/to-importFn.ts → lib/core-webpack/src/to-importFn.ts

@@ -1,7 +1,8 @@
 import dedent from 'ts-dedent';
+import type { NormalizedStoriesSpecifier } from '@storybook/core-common';
+import { globToRegexp } from '@storybook/core-common';
 
-import type { NormalizedStoriesSpecifier } from '../types';
-import { globToRegexp } from './glob-to-regexp';
+import { importPipeline } from './importPipeline';
 
 export function webpackIncludeRegexp(specifier: NormalizedStoriesSpecifier) {
   const { directory, files } = specifier;
@@ -41,15 +42,28 @@ export function toImportFnPart(specifier: NormalizedStoriesSpecifier) {
   `;
 }
 
-export function toImportFn(stories: NormalizedStoriesSpecifier[]) {
+export function toImportFn(
+  stories: NormalizedStoriesSpecifier[],
+  { needPipelinedImport }: { needPipelinedImport?: boolean } = {}
+) {
+  let pipelinedImport = `const pipeline = (x) => x();`;
+  if (needPipelinedImport) {
+    pipelinedImport = `
+      const importPipeline = ${importPipeline};
+      const pipeline = importPipeline();
+    `;
+  }
+
   return dedent`
+    ${pipelinedImport}
+
     const importers = [
       ${stories.map(toImportFnPart).join(',\n')}
     ];
 
     export async function importFn(path) {
       for (let i = 0; i < importers.length; i++) {
-        const moduleExports = await importers[i](path);
+        const moduleExports = await pipeline(() => importers[i](path));
         if (moduleExports) {
           return moduleExports;
         }

lib/core-common/src/utils/__tests__/to-require-context.test.ts → lib/core-webpack/src/to-require-context.test.ts

@@ -1,6 +1,7 @@
 import path from 'path';
-import { toRequireContext } from '../to-require-context';
-import { normalizeStoriesEntry } from '../normalize-stories';
+import { normalizeStoriesEntry } from '@storybook/core-common';
+
+import { toRequireContext } from './to-require-context';
 
 const testCases = [
   {

lib/core-common/src/utils/to-require-context.ts → lib/core-webpack/src/to-require-context.ts

@@ -1,5 +1,5 @@
-import { NormalizedStoriesSpecifier } from '../types';
-import { globToRegexp } from './glob-to-regexp';
+import type { NormalizedStoriesSpecifier } from '@storybook/core-common';
+import { globToRegexp } from '@storybook/core-common';
 
 export const toRequireContext = (specifier: NormalizedStoriesSpecifier) => {
   const { directory, files } = specifier;

yarn.lock

@@ -6975,6 +6975,7 @@ __metadata:
     "@storybook/client-api": 7.0.0-alpha.5
     "@storybook/core-client": 7.0.0-alpha.5
     "@storybook/core-common": 7.0.0-alpha.5
+    "@storybook/core-webpack": 7.0.0-alpha.5
     "@storybook/csf": 0.0.2--canary.4566f4d.1
     "@storybook/react": 7.0.0-alpha.5
     "@storybook/vue": 7.0.0-alpha.5