Skip to content

Commit

Permalink
Merge pull request #27016 from heyimalex/init-performance-fix
Browse files Browse the repository at this point in the history 
Core: Fix startup hang caused by watchStorySpecifiers
  • Loading branch information
@shilman
shilman authored Jun 13, 2024
2 parents ce60b0e + 80f8dc0 commit dd69fd3
Show file tree
Hide file tree
Showing 3 changed files with 104 additions and 44 deletions.
35 changes: 25 additions & 10 deletions code/lib/core-server/src/utils/stories-json.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -356,12 +356,17 @@ describe('useStoriesJson', () => {

expect(Watchpack).toHaveBeenCalledTimes(1);
const watcher = Watchpack.mock.instances[0];
expect(watcher.watch).toHaveBeenCalledWith({ directories: ['./src'] });
expect(watcher.watch).toHaveBeenCalledWith(
expect.objectContaining({
directories: expect.any(Array),
files: expect.any(Array),
})
);

expect(watcher.on).toHaveBeenCalledTimes(2);
const onChange = watcher.on.mock.calls[0][1];

await onChange('src/nested/Button.stories.ts');
await onChange(`${workingDir}/src/nested/Button.stories.ts`);
expect(mockServerChannel.emit).toHaveBeenCalledTimes(1);
expect(mockServerChannel.emit).toHaveBeenCalledWith(STORY_INDEX_INVALIDATED);
});
Expand Down Expand Up @@ -389,12 +394,17 @@ describe('useStoriesJson', () => {

expect(Watchpack).toHaveBeenCalledTimes(1);
const watcher = Watchpack.mock.instances[0];
expect(watcher.watch).toHaveBeenCalledWith({ directories: ['./src'] });
expect(watcher.watch).toHaveBeenCalledWith(
expect.objectContaining({
directories: expect.any(Array),
files: expect.any(Array),
})
);

expect(watcher.on).toHaveBeenCalledTimes(2);
const onChange = watcher.on.mock.calls[0][1];

await onChange('src/nested/Button.stories.ts');
await onChange(`${workingDir}/src/nested/Button.stories.ts`);
expect(mockServerChannel.emit).toHaveBeenCalledTimes(1);
expect(mockServerChannel.emit).toHaveBeenCalledWith(STORY_INDEX_INVALIDATED);
});
Expand Down Expand Up @@ -423,16 +433,21 @@ describe('useStoriesJson', () => {

expect(Watchpack).toHaveBeenCalledTimes(1);
const watcher = Watchpack.mock.instances[0];
expect(watcher.watch).toHaveBeenCalledWith({ directories: ['./src'] });
expect(watcher.watch).toHaveBeenCalledWith(
expect.objectContaining({
directories: expect.any(Array),
files: expect.any(Array),
})
);

expect(watcher.on).toHaveBeenCalledTimes(2);
const onChange = watcher.on.mock.calls[0][1];

await onChange('src/nested/Button.stories.ts');
await onChange('src/nested/Button.stories.ts');
await onChange('src/nested/Button.stories.ts');
await onChange('src/nested/Button.stories.ts');
await onChange('src/nested/Button.stories.ts');
await onChange(`${workingDir}/src/nested/Button.stories.ts`);
await onChange(`${workingDir}/src/nested/Button.stories.ts`);
await onChange(`${workingDir}/src/nested/Button.stories.ts`);
await onChange(`${workingDir}/src/nested/Button.stories.ts`);
await onChange(`${workingDir}/src/nested/Button.stories.ts`);

expect(mockServerChannel.emit).toHaveBeenCalledTimes(1);
expect(mockServerChannel.emit).toHaveBeenCalledWith(STORY_INDEX_INVALIDATED);
Expand Down
47 changes: 37 additions & 10 deletions code/lib/core-server/src/utils/watch-story-specifiers.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ describe('watchStorySpecifiers', () => {
configDir: path.join(workingDir, '.storybook'),
workingDir,
};
const abspath = (filename: string) => path.join(workingDir, filename);

let close: () => void;
afterEach(() => close?.());
Expand All @@ -25,11 +26,18 @@ describe('watchStorySpecifiers', () => {

expect(Watchpack).toHaveBeenCalledTimes(1);
const watcher = Watchpack.mock.instances[0];
expect(watcher.watch).toHaveBeenCalledWith({ directories: ['./src'] });
expect(watcher.watch).toHaveBeenCalledWith(
expect.objectContaining({
directories: expect.any(Array),
files: expect.any(Array),
})
);

expect(watcher.on).toHaveBeenCalledTimes(2);
const onChange = watcher.on.mock.calls[0][1];
const onRemove = watcher.on.mock.calls[1][1];
const baseOnChange = watcher.on.mock.calls[0][1];
const baseOnRemove = watcher.on.mock.calls[1][1];
const onChange = (filename: string, ...args: any[]) => baseOnChange(abspath(filename), ...args);
const onRemove = (filename: string, ...args: any[]) => baseOnRemove(abspath(filename), ...args);

// File changed, matching
onInvalidate.mockClear();
Expand Down Expand Up @@ -72,10 +80,16 @@ describe('watchStorySpecifiers', () => {

expect(Watchpack).toHaveBeenCalledTimes(1);
const watcher = Watchpack.mock.instances[0];
expect(watcher.watch).toHaveBeenCalledWith({ directories: ['./src'] });
expect(watcher.watch).toHaveBeenCalledWith(
expect.objectContaining({
directories: expect.any(Array),
files: expect.any(Array),
})
);

expect(watcher.on).toHaveBeenCalledTimes(2);
const onChange = watcher.on.mock.calls[0][1];
const baseOnChange = watcher.on.mock.calls[0][1];
const onChange = (filename: string, ...args: any[]) => baseOnChange(abspath(filename), ...args);

onInvalidate.mockClear();
await onChange('src/nested', 1234);
Expand All @@ -90,11 +104,18 @@ describe('watchStorySpecifiers', () => {

expect(Watchpack).toHaveBeenCalledTimes(1);
const watcher = Watchpack.mock.instances[0];
expect(watcher.watch).toHaveBeenCalledWith({ directories: ['./src/nested'] });
expect(watcher.watch).toHaveBeenCalledWith(
expect.objectContaining({
directories: expect.any(Array),
files: expect.any(Array),
})
);

expect(watcher.on).toHaveBeenCalledTimes(2);
const onChange = watcher.on.mock.calls[0][1];
const onRemove = watcher.on.mock.calls[1][1];
const baseOnChange = watcher.on.mock.calls[0][1];
const baseOnRemove = watcher.on.mock.calls[1][1];
const onChange = (filename: string, ...args: any[]) => baseOnChange(abspath(filename), ...args);
const onRemove = (filename: string, ...args: any[]) => baseOnRemove(abspath(filename), ...args);

// File changed, matching
onInvalidate.mockClear();
Expand Down Expand Up @@ -131,10 +152,16 @@ describe('watchStorySpecifiers', () => {

expect(Watchpack).toHaveBeenCalledTimes(1);
const watcher = Watchpack.mock.instances[0];
expect(watcher.watch).toHaveBeenCalledWith({ directories: ['./src', './src/nested'] });
expect(watcher.watch).toHaveBeenCalledWith(
expect.objectContaining({
directories: expect.any(Array),
files: expect.any(Array),
})
);

expect(watcher.on).toHaveBeenCalledTimes(2);
const onChange = watcher.on.mock.calls[0][1];
const baseOnChange = watcher.on.mock.calls[0][1];
const onChange = (filename: string, ...args: any[]) => baseOnChange(abspath(filename), ...args);

onInvalidate.mockClear();
await onChange('src/nested/Button.stories.ts', 1234);
Expand Down
66 changes: 42 additions & 24 deletions code/lib/core-server/src/utils/watch-story-specifiers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import Watchpack from 'watchpack';
import slash from 'slash';
import fs from 'fs';
import path from 'path';
import uniq from 'lodash/uniq.js';

import type { NormalizedStoriesSpecifier, Path } from '@storybook/types';
import { commonGlobOptions } from '@storybook/core-common';
Expand All @@ -15,34 +14,58 @@ const isDirectory = (directory: Path) => {
}
};

// Watchpack (and path.relative) passes paths either with no leading './' - e.g. `src/Foo.stories.js`,
// or with a leading `../` (etc), e.g. `../src/Foo.stories.js`.
// We want to deal in importPaths relative to the working dir, so we normalize
function toImportPath(relativePath: Path) {
return relativePath.startsWith('.') ? relativePath : `./${relativePath}`;
// Takes an array of absolute paths to directories and synchronously returns
// absolute paths to all existing files and directories nested within those
// directories (including the passed parent directories).
function getNestedFilesAndDirectories(directories: Path[]) {
const traversedDirectories = new Set<Path>();
const files = new Set<Path>();
const traverse = (directory: Path) => {
if (traversedDirectories.has(directory)) {
return;
}
fs.readdirSync(directory, { withFileTypes: true }).forEach((ent: fs.Dirent) => {
if (ent.isDirectory()) {
traverse(path.join(directory, ent.name));
} else if (ent.isFile()) {
files.add(path.join(directory, ent.name));
}
});
traversedDirectories.add(directory);
};
directories.filter(isDirectory).forEach(traverse);
return { files: Array.from(files), directories: Array.from(traversedDirectories) };
}

export function watchStorySpecifiers(
specifiers: NormalizedStoriesSpecifier[],
options: { workingDir: Path },
onInvalidate: (specifier: NormalizedStoriesSpecifier, path: Path, removed: boolean) => void
) {
// Watch all nested files and directories up front to avoid this issue:
// https://github.com/webpack/watchpack/issues/222
const { files, directories } = getNestedFilesAndDirectories(
specifiers.map((ns) => path.resolve(options.workingDir, ns.directory))
);

// See https://www.npmjs.com/package/watchpack for full options.
// If you want less traffic, consider using aggregation with some interval
const wp = new Watchpack({
// poll: true, // Slow!!! Enable only in special cases
followSymlinks: false,
ignored: ['**/.git', '**/node_modules'],
});
wp.watch({
directories: uniq(specifiers.map((ns) => ns.directory)),
});
wp.watch({ files, directories });

const toImportPath = (absolutePath: Path) => {
const relativePath = path.relative(options.workingDir, absolutePath);
return slash(relativePath.startsWith('.') ? relativePath : `./${relativePath}`);
};

async function onChangeOrRemove(watchpackPath: Path, removed: boolean) {
// Watchpack passes paths either with no leading './' - e.g. `src/Foo.stories.js`,
// or with a leading `../` (etc), e.g. `../src/Foo.stories.js`.
// We want to deal in importPaths relative to the working dir, or absolute paths.
const importPath = slash(watchpackPath.startsWith('.') ? watchpackPath : `./${watchpackPath}`);
async function onChangeOrRemove(absolutePath: Path, removed: boolean) {
// Watchpack should return absolute paths, given we passed in absolute paths
// to watch. Convert to an import path so we can run against the specifiers.
const importPath = toImportPath(absolutePath);

const matchingSpecifier = specifiers.find((ns) => ns.importPathMatcher.exec(importPath));
if (matchingSpecifier) {
Expand All @@ -55,7 +78,6 @@ export function watchStorySpecifiers(
// However, when a directory is added, it does not fire events for any files *within* the directory,
// so we need to scan within that directory for new files. It is tricky to use a glob for this,
// so we'll do something a bit more "dumb" for now
const absolutePath = path.join(options.workingDir, importPath);
if (!removed && isDirectory(absolutePath)) {
await Promise.all(
specifiers
Expand All @@ -66,25 +88,21 @@ export function watchStorySpecifiers(
// If `./path/to/dir` was added, check all files matching `./path/to/dir/**/*.stories.*`
// (where the last bit depends on `files`).
const dirGlob = path.join(
options.workingDir,
importPath,
absolutePath,
'**',
// files can be e.g. '**/foo/*/*.js' so we just want the last bit,
// because the directoru could already be within the files part (e.g. './x/foo/bar')
// because the directory could already be within the files part (e.g. './x/foo/bar')
path.basename(specifier.files)
);

// Dynamically import globby because it is a pure ESM module
const { globby } = await import('globby');

// glob only supports forward slashes
const files = await globby(slash(dirGlob), commonGlobOptions(dirGlob));
const addedFiles = await globby(slash(dirGlob), commonGlobOptions(dirGlob));

files.forEach((filePath) => {
const fileImportPath = toImportPath(
// use posix path separators even on windows
path.relative(options.workingDir, filePath).replace(/\\/g, '/')
);
addedFiles.forEach((filePath: Path) => {
const fileImportPath = toImportPath(filePath);

if (specifier.importPathMatcher.exec(fileImportPath)) {
onInvalidate(specifier, fileImportPath, removed);
Expand Down

0 comments on commit dd69fd3

Please sign in to comment.