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.

Sign up for GitHub

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

Jump to bottom

Core: Fix startup hang caused by watchStorySpecifiers #27016

Merged
merged 7 commits into from
Jun 13, 2024
Merged

Core: Fix startup hang caused by watchStorySpecifiers #27016

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
dc4f4d3
Prevent long hang on startup
heyimalex May 2, 2024
5f698d2
fixes
heyimalex May 2, 2024
1450b49
shadowing fix
heyimalex May 2, 2024
d750423
howd this slip through
heyimalex May 2, 2024
17f9b72
Fixup tests
heyimalex May 2, 2024
b6e3747
fixup stores json test
heyimalex May 8, 2024
80f8dc0
Merge branch 'next' into init-performance-fix
shilman Jun 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 25 additions & 10 deletions code/lib/core-server/src/utils/stories-json.test.ts
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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).
Comment on lines +17 to +19
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be sync? If we do it async might it give other parts of our boot up a chance to do work while waiting on FS?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it must be sync, we'd just create the watcher asynchronously after directory walking completes. IMO performance implications here are not significant tho, even with 10k directories readdirSync calls seem to complete in negligible time.

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