From dc4f4d300a702a6e567e894b908673023d44a3fd Mon Sep 17 00:00:00 2001 From: Alex Guerra Date: Thu, 2 May 2024 13:03:35 -0400 Subject: [PATCH 1/6] Prevent long hang on startup --- .../src/utils/watch-story-specifiers.ts | 61 ++++++++++++------- 1 file changed, 39 insertions(+), 22 deletions(-) diff --git a/code/lib/core-server/src/utils/watch-story-specifiers.ts b/code/lib/core-server/src/utils/watch-story-specifiers.ts index 70ec0d4eb7fb..dc5a0a9c503d 100644 --- a/code/lib/core-server/src/utils/watch-story-specifiers.ts +++ b/code/lib/core-server/src/utils/watch-story-specifiers.ts @@ -15,11 +15,27 @@ 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(); + const files = new Set(); + 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( @@ -27,6 +43,10 @@ export function watchStorySpecifiers( 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({ @@ -34,15 +54,17 @@ export function watchStorySpecifiers( 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) { @@ -55,7 +77,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 @@ -66,11 +87,10 @@ 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) ); @@ -80,11 +100,8 @@ export function watchStorySpecifiers( // glob only supports forward slashes const files = 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, '/') - ); + files.forEach((filePath: Path) => { + const fileImportPath = toImportPath(filePath); if (specifier.importPathMatcher.exec(fileImportPath)) { onInvalidate(specifier, fileImportPath, removed); From 5f698d26b10f4c779ab319aa81491797d7156426 Mon Sep 17 00:00:00 2001 From: Alex Guerra Date: Thu, 2 May 2024 13:34:11 -0400 Subject: [PATCH 2/6] fixes --- .../src/utils/watch-story-specifiers.ts | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/code/lib/core-server/src/utils/watch-story-specifiers.ts b/code/lib/core-server/src/utils/watch-story-specifiers.ts index dc5a0a9c503d..7418886b3508 100644 --- a/code/lib/core-server/src/utils/watch-story-specifiers.ts +++ b/code/lib/core-server/src/utils/watch-story-specifiers.ts @@ -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'; @@ -25,17 +24,17 @@ function getNestedFilesAndDirectories(directories: Path[]) { if (traversedDirectories.has(directory)) { return; } - fs.readdirSync(directory, { withFileTypes: true}).forEach((ent: fs.Dirent) => { - if (ent.isDirectory) { + fs.readdirSync(directory, { withFileTypes: true }).forEach((ent: fs.Dirent) => { + if (ent.isDirectory()) { traverse(path.join(directory, ent.name)); - } else if (ent.isFile) { + } 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) } + return { files: Array.from(files), directories: Array.from(traversedDirectories) }; } export function watchStorySpecifiers( @@ -45,7 +44,9 @@ export function watchStorySpecifiers( ) { // 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))) + 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 @@ -59,7 +60,7 @@ export function watchStorySpecifiers( const toImportPath = (absolutePath: Path) => { const relativePath = path.relative(options.workingDir, absolutePath); return slash(relativePath.startsWith('.') ? relativePath : `./${relativePath}`); - } + }; async function onChangeOrRemove(absolutePath: Path, removed: boolean) { // Watchpack should return absolute paths, given we passed in absolute paths From 1450b495ebc310c9ffd6fbd24b258b5d8bb73ae7 Mon Sep 17 00:00:00 2001 From: Alex Guerra Date: Thu, 2 May 2024 13:49:53 -0400 Subject: [PATCH 3/6] shadowing fix --- code/lib/core-server/src/utils/watch-story-specifiers.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/code/lib/core-server/src/utils/watch-story-specifiers.ts b/code/lib/core-server/src/utils/watch-story-specifiers.ts index 7418886b3508..ddb219effc98 100644 --- a/code/lib/core-server/src/utils/watch-story-specifiers.ts +++ b/code/lib/core-server/src/utils/watch-story-specifiers.ts @@ -44,6 +44,7 @@ export function watchStorySpecifiers( ) { // Watch all nested files and directories up front to avoid this issue: // https://github.com/webpack/watchpack/issues/222 + const absDirs = specifiers.map((ns) => path.resolve(options.workingDir, ns.directory)); const { files, directories } = getNestedFilesAndDirectories( specifiers.map((ns) => path.resolve(options.workingDir, ns.directory)) ); @@ -99,9 +100,9 @@ export function watchStorySpecifiers( 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: Path) => { + addedFiles.forEach((filePath: Path) => { const fileImportPath = toImportPath(filePath); if (specifier.importPathMatcher.exec(fileImportPath)) { From d750423e59de31b542bf617031630f9164a87d2e Mon Sep 17 00:00:00 2001 From: Alex Guerra Date: Thu, 2 May 2024 14:06:18 -0400 Subject: [PATCH 4/6] howd this slip through --- code/lib/core-server/src/utils/watch-story-specifiers.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/code/lib/core-server/src/utils/watch-story-specifiers.ts b/code/lib/core-server/src/utils/watch-story-specifiers.ts index ddb219effc98..414fd4c87617 100644 --- a/code/lib/core-server/src/utils/watch-story-specifiers.ts +++ b/code/lib/core-server/src/utils/watch-story-specifiers.ts @@ -44,7 +44,6 @@ export function watchStorySpecifiers( ) { // Watch all nested files and directories up front to avoid this issue: // https://github.com/webpack/watchpack/issues/222 - const absDirs = specifiers.map((ns) => path.resolve(options.workingDir, ns.directory)); const { files, directories } = getNestedFilesAndDirectories( specifiers.map((ns) => path.resolve(options.workingDir, ns.directory)) ); From 17f9b724a0ba1c4ba7bd5a290abcab3495af0536 Mon Sep 17 00:00:00 2001 From: Alex Guerra Date: Thu, 2 May 2024 14:34:07 -0400 Subject: [PATCH 5/6] Fixup tests --- .../src/utils/watch-story-specifiers.test.ts | 47 +++++++++++++++---- 1 file changed, 37 insertions(+), 10 deletions(-) diff --git a/code/lib/core-server/src/utils/watch-story-specifiers.test.ts b/code/lib/core-server/src/utils/watch-story-specifiers.test.ts index 22d3e0901de8..c12df571e425 100644 --- a/code/lib/core-server/src/utils/watch-story-specifiers.test.ts +++ b/code/lib/core-server/src/utils/watch-story-specifiers.test.ts @@ -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?.()); @@ -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(); @@ -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); @@ -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(); @@ -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); From b6e37475cf1b8470fe413dd3103bca3205fd8f21 Mon Sep 17 00:00:00 2001 From: Alex Guerra Date: Wed, 8 May 2024 14:25:01 -0400 Subject: [PATCH 6/6] fixup stores json test --- .../src/utils/stories-json.test.ts | 35 +++++++++++++------ 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/code/lib/core-server/src/utils/stories-json.test.ts b/code/lib/core-server/src/utils/stories-json.test.ts index a621f5443ce3..1d537fc2650b 100644 --- a/code/lib/core-server/src/utils/stories-json.test.ts +++ b/code/lib/core-server/src/utils/stories-json.test.ts @@ -303,12 +303,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); }); @@ -336,12 +341,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); }); @@ -370,16 +380,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);