feat(cli): restore --watch flag for build-storybook command#34200
Closed
tysoncung wants to merge 2 commits into
Closed
feat(cli): restore --watch flag for build-storybook command#34200tysoncung wants to merge 2 commits into
tysoncung wants to merge 2 commits into
Conversation
Re-implement the --watch / -w flag for 'storybook build' that was previously removed due to being broken. Changes: - Add -w/--watch CLI option to the build command - Add 'watch' property to CLIOptions type - Implement webpack watch mode using compiler.watch() instead of compiler.run() when --watch is passed - Implement vite watch mode by setting build.watch option - Handle rebuild logging and error recovery in watch mode - Prevent premature process exit and 'build completed' message in watch mode The watch flag was originally added via storybookjs#2762 but regressed at some point when the build pipeline was refactored. The CLI option was later removed entirely in storybookjs#16165 since it wasn't functional. This restores the feature by properly wiring the watch option through to both the webpack5 and vite builders. Fixes storybookjs#15946
Contributor
📝 WalkthroughWalkthroughAdds a watch mode for Storybook builds: a new CLI flag ( Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Core
participant BuilderSelector
participant ViteBuilder
participant WebpackBuilder
participant Compiler
CLI->>Core: run build (--watch?)
Core->>BuilderSelector: select builder (vite|webpack) with options.watch
alt Vite chosen
BuilderSelector->>ViteBuilder: invoke build(finalConfig with build.watch if watch)
ViteBuilder->>ViteBuilder: start Vite build (watch enabled or not)
ViteBuilder-->>Core: build completed / watching
else Webpack chosen
BuilderSelector->>WebpackBuilder: start build(options.watch)
alt watch=true
WebpackBuilder->>Compiler: compiler.watch(...)
Compiler-->>WebpackBuilder: initial build result / errors
WebpackBuilder->>Core: log first build result (error => fail on first build)
loop on changes
Compiler-->>WebpackBuilder: incremental rebuild event (stats/errors)
WebpackBuilder->>Core: log rebuild result (warnings/errors), continue watching
end
else
WebpackBuilder->>Compiler: compiler.run()
Compiler-->>WebpackBuilder: stats or error
WebpackBuilder->>Core: log result and close compiler
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can customize the tone of the review comments and chat replies.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
code/builders/builder-webpack5/src/index.ts (1)
311-314: Rebuild duration is currently cumulative, not per rebuildAt Line 311,
printDuration(startTime)measures time from the initial build start, so later rebuild logs overstate rebuild duration. Consider tracking a per-rebuild start timestamp.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/builders/builder-webpack5/src/index.ts` around lines 311 - 314, The rebuild log uses printDuration(startTime) which measures from the original build start and thus reports cumulative time; change the code to record a fresh per-rebuild start timestamp inside the rebuild handler (e.g., create a new variable like rebuildStart or perRebuildStart at the top of the watcher/rebuild callback) and pass that timestamp to printDuration instead of the existing startTime; update any related references in the watcher/rebuild closure (where logger.info(`Rebuild completed in ${printDuration(startTime)}`) is called) so each rebuild logs its own duration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/builders/builder-vite/src/build.ts`:
- Around line 94-99: The current watch enablement in build.ts overwrites any
existing finalConfig.build.watch from viteFinal by setting watch: {}, so
preserve and merge existing settings instead: when options.watch is true, set
finalConfig.build.watch to the existing object (finalConfig.build.watch) or
merge into it rather than replacing it, e.g. assign/merge into
finalConfig.build.watch so user/framework watch options are retained; update the
block that references finalConfig, options.watch and build.watch in build.ts
accordingly.
In `@code/builders/builder-webpack5/src/index.ts`:
- Around line 322-324: Inside the compiler.run() callback where the code
currently does "if (!stats) { throw new WebpackMissingStatsError(); }", replace
the thrown exception with a call to the existing fail() helper (e.g., fail(new
WebpackMissingStatsError())) so the promise is rejected instead of throwing an
uncaught exception; ensure you call return after fail() or otherwise stop
further processing in that callback (mirror the pattern used in the watch path
and other non-watch error branches where fail(...) is used).
In `@code/core/src/core-server/build-static.ts`:
- Around line 175-177: The log incorrectly reports "Building preview..." when
options.watch is true even if preview is skipped; update the conditional around
logger.info (in build-static.ts where options.watch and options.ignorePreview
are checked) to first check options.ignorePreview (or require
!options.ignorePreview) before logging, e.g. only call logger.info('Building
preview in watch mode..') when options.watch && !options.ignorePreview so the
message is suppressed when preview is disabled.
---
Nitpick comments:
In `@code/builders/builder-webpack5/src/index.ts`:
- Around line 311-314: The rebuild log uses printDuration(startTime) which
measures from the original build start and thus reports cumulative time; change
the code to record a fresh per-rebuild start timestamp inside the rebuild
handler (e.g., create a new variable like rebuildStart or perRebuildStart at the
top of the watcher/rebuild callback) and pass that timestamp to printDuration
instead of the existing startTime; update any related references in the
watcher/rebuild closure (where logger.info(`Rebuild completed in
${printDuration(startTime)}`) is called) so each rebuild logs its own duration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c389ba71-73ae-47c2-a969-fd895ae1be6e
📒 Files selected for processing (5)
code/builders/builder-vite/src/build.tscode/builders/builder-webpack5/src/index.tscode/core/src/bin/core.tscode/core/src/core-server/build-static.tscode/core/src/types/modules/core-common.ts
- Preserve existing build.watch config in Vite builder - Use fail() instead of throw in compiler.run() callback for proper promise rejection - Fix conditional order to check ignorePreview before watch in build-static.ts - Track per-rebuild timestamp for accurate rebuild duration logging
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/builders/builder-webpack5/src/index.ts (1)
271-317: Consider storing theWatchinginstance for graceful shutdown.The
compiler.watch()call returns aWatchinginstance that isn't being stored. While process termination (Ctrl+C) will clean up resources, storing this reference would enable graceful shutdown via thebailfunction — similar to howcompilationis tracked in thestarterfunction.This is acceptable for the current CLI-driven use case but worth noting for future programmatic usage.
♻️ Potential enhancement for watcher cleanup
+let watching: ReturnType<typeof compiler.watch> | undefined; + if (options.watch) { let isFirstBuild = true; let rebuildStartTime = startTime; logger.info('Watching for changes...'); - compiler.watch(config.watchOptions || {}, (error, stats) => { + watching = compiler.watch(config.watchOptions || {}, (error, stats) => {Then update the
bailfunction to closewatchingwhen present. This could be addressed in a follow-up PR if needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/builders/builder-webpack5/src/index.ts` around lines 271 - 317, The watcher returned by compiler.watch is not stored; capture its Watching instance (e.g., const watching = compiler.watch(...)) so you can close it on shutdown, and modify the bail function to call watching.close() (if watching is defined) for graceful shutdown; ensure the watching variable is accessible to the same scope as bail (mirror how compilation is tracked in starter) and keep existing first-build logic (isFirstBuild, succeed, fail) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/builders/builder-webpack5/src/index.ts`:
- Around line 271-317: The watcher returned by compiler.watch is not stored;
capture its Watching instance (e.g., const watching = compiler.watch(...)) so
you can close it on shutdown, and modify the bail function to call
watching.close() (if watching is defined) for graceful shutdown; ensure the
watching variable is accessible to the same scope as bail (mirror how
compilation is tracked in starter) and keep existing first-build logic
(isFirstBuild, succeed, fail) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b3736a2d-e013-4762-9296-0a3dd9055b1b
📒 Files selected for processing (3)
code/builders/builder-vite/src/build.tscode/builders/builder-webpack5/src/index.tscode/core/src/core-server/build-static.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- code/core/src/core-server/build-static.ts
- code/builders/builder-vite/src/build.ts
Contributor
|
There is already a PR open which attempts to fix this: #33482 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Restores the
--watchflag forbuild-storybookthat was removed in #16165.Problem
The
--watchoption was originally added in #2762 but regressed during build pipeline refactors. Rather than fix it, #16165 removed the flag entirely. Users have been requesting this feature back (28+ reactions on #15946).Solution
This PR re-adds the
-w, --watchflag and properly wires it through to both builders:Webpack5 Builder
compiler.watch()instead ofcompiler.run()whenoptions.watchis trueVite Builder
build.watch: {}on the vite config whenoptions.watchis trueFiles Changed
code/core/src/bin/core.ts— Added-w, --watchoption to build commandcode/core/src/types/modules/core-common.ts— Addedwatch?: booleanto CLIOptionscode/core/src/core-server/build-static.ts— Watch mode loggingcode/builders/builder-webpack5/src/index.ts— Webpack watch implementationcode/builders/builder-vite/src/build.ts— Vite watch implementationUsage
Closes #15946
Summary by CodeRabbit