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

Webpack5: Don't import a second file while a first one is in flight #18432

Merged
9 changes: 5 additions & 4 deletions lib/builder-webpack5/src/preview/iframe-webpack.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
tmeasday marked this conversation as resolved.
Show resolved Hide resolved
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';

Expand Down Expand Up @@ -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;
yannbf marked this conversation as resolved.
Show resolved Hide resolved
virtualModuleMapping[storiesPath] = toImportFn(stories, { needPipelinedImport });
const configEntryPath = path.resolve(path.join(workingDir, 'storybook-config-entry.js'));
virtualModuleMapping[configEntryPath] = handlebars(
await readTemplate(
Expand Down
3 changes: 1 addition & 2 deletions lib/core-common/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down
121 changes: 121 additions & 0 deletions lib/core-webpack/src/importPipeline.test.ts
Original file line number Diff line number Diff line change
@@ -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');
});
22 changes: 22 additions & 0 deletions lib/core-webpack/src/importPipeline.ts
Original file line number Diff line number Diff line change
@@ -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;
};
}
2 changes: 2 additions & 0 deletions lib/core-webpack/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Original file line number Diff line number Diff line change
@@ -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[]][] = [
[
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -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 = [
{
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
Expand Down