fix(lib): reload config on watch restart#506
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
d6d3012 to
08412f6
Compare
|
rolldown watch mode is not working on Github Action CI env, but I run this snap-test on local it was worked. Skip it for now. |
Pass the build function as the restart callback to `buildWithConfigs` instead of an empty function, ensuring config is re-resolved when watch mode detects config file changes. Ref: rolldown/tsdown@1756b03
Add an `after` field to steps.json that runs cleanup commands after the test completes, regardless of success or failure. This is useful for killing background processes that would otherwise hang. The after commands are not included in the snap output.
08412f6 to
825a754
Compare
There was a problem hiding this comment.
Pull request overview
Updates the vite lib CLI watch restart behavior so that config is re-resolved when a watched config file change triggers a restart, and extends the snapshot test runner to support per-command timeouts and always-run cleanup commands.
Changes:
- Pass the build function as the watch-restart callback to
buildWithConfigsso restarts re-resolve config. - Add
timeoutper command andaftercleanup commands to the snap-test runner. - Add a (currently todo) snap-test fixture for verifying config reload/restart behavior in watch mode.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/tools/src/snap-test.ts | Adds Command.timeout and Steps.after, and runs after commands in a finally block. |
| packages/cli/src/lib-bin.ts | Wraps build in runBuild() and passes it as the restart callback to re-resolve config on watch restarts. |
| packages/cli/snap-tests-todo/command-lib-watch-restart/wait-for-dist2.sh | Adds helper script to wait for dist2 after config change. |
| packages/cli/snap-tests-todo/command-lib-watch-restart/wait-for-dist.sh | Adds helper script to wait for initial dist. |
| packages/cli/snap-tests-todo/command-lib-watch-restart/vite.config.ts | Adds minimal config used by the watch-restart snapshot fixture. |
| packages/cli/snap-tests-todo/command-lib-watch-restart/steps.json | Defines the watch-restart scenario, including a longer timeout and cleanup via after. |
| packages/cli/snap-tests-todo/command-lib-watch-restart/src/index.ts | Adds minimal source input for the fixture. |
| packages/cli/snap-tests-todo/command-lib-watch-restart/snap.txt | Adds expected snapshot output for the fixture. |
| packages/cli/snap-tests-todo/command-lib-watch-restart/run-watch.sh | Starts vite lib --watch in background and writes PID/log file. |
| packages/cli/snap-tests-todo/command-lib-watch-restart/package.json | Declares the fixture package metadata. |
| packages/cli/snap-tests-todo/command-lib-watch-restart/kill-watch.sh | Adds cleanup script to stop the background watch process tree. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const outputStream = await open(outputStreamPath, 'w'); | ||
|
|
||
| const exitCode = await Promise.race([ | ||
| execute(stripComments(cmd.command), [], { | ||
| env, | ||
| cwd, | ||
| stdin: null, | ||
| // Declared to be `Writable` but `FileHandle` works too. | ||
| // @ts-expect-error | ||
| stderr: outputStream, | ||
| // @ts-expect-error | ||
| stdout: outputStream, | ||
| glob: { | ||
| // Disable glob expansion. Pass args like '--filter=*' as-is. | ||
| isGlobPattern: () => false, | ||
| match: async () => [], | ||
| }, | ||
| }), | ||
| setTimeout(cmd.timeout ?? 30 * 1000), | ||
| ]); | ||
|
|
||
| await outputStream.close(); | ||
|
|
||
| let output = readFileSync(outputStreamPath, 'utf-8'); | ||
|
|
||
| let commandLine = `> ${cmd.command}`; | ||
| if (exitCode !== 0) { | ||
| commandLine = (exitCode === undefined ? '[timeout]' : `[${exitCode}]`) + commandLine; | ||
| } else { | ||
| // only allow ignore output if the command is successful | ||
| if (cmd.ignoreOutput) { | ||
| output = ''; | ||
| } | ||
| } | ||
| newSnap.push(commandLine); | ||
| if (output.length > 0) { | ||
| newSnap.push(replaceUnstableOutput(output, caseTmpDir)); | ||
| } | ||
| if (exitCode === undefined) { | ||
| break; // Stop executing further commands on timeout | ||
| } |
There was a problem hiding this comment.
outputStream is only closed on the success path. If execute(...) rejects (e.g., command not found / spawn error) or Promise.race rejects, the function will jump to the outer finally and leak the open file descriptor for output.log. Consider wrapping each command execution in a per-command try/finally that always closes outputStream (and ideally still reads any partial output).
| const outputStream = await open(outputStreamPath, 'w'); | |
| const exitCode = await Promise.race([ | |
| execute(stripComments(cmd.command), [], { | |
| env, | |
| cwd, | |
| stdin: null, | |
| // Declared to be `Writable` but `FileHandle` works too. | |
| // @ts-expect-error | |
| stderr: outputStream, | |
| // @ts-expect-error | |
| stdout: outputStream, | |
| glob: { | |
| // Disable glob expansion. Pass args like '--filter=*' as-is. | |
| isGlobPattern: () => false, | |
| match: async () => [], | |
| }, | |
| }), | |
| setTimeout(cmd.timeout ?? 30 * 1000), | |
| ]); | |
| await outputStream.close(); | |
| let output = readFileSync(outputStreamPath, 'utf-8'); | |
| let commandLine = `> ${cmd.command}`; | |
| if (exitCode !== 0) { | |
| commandLine = (exitCode === undefined ? '[timeout]' : `[${exitCode}]`) + commandLine; | |
| } else { | |
| // only allow ignore output if the command is successful | |
| if (cmd.ignoreOutput) { | |
| output = ''; | |
| } | |
| } | |
| newSnap.push(commandLine); | |
| if (output.length > 0) { | |
| newSnap.push(replaceUnstableOutput(output, caseTmpDir)); | |
| } | |
| if (exitCode === undefined) { | |
| break; // Stop executing further commands on timeout | |
| } | |
| let outputStream: fsPromises.FileHandle | null = null; | |
| try { | |
| outputStream = await open(outputStreamPath, 'w'); | |
| const exitCode = await Promise.race([ | |
| execute(stripComments(cmd.command), [], { | |
| env, | |
| cwd, | |
| stdin: null, | |
| // Declared to be `Writable` but `FileHandle` works too. | |
| // @ts-expect-error | |
| stderr: outputStream, | |
| // @ts-expect-error | |
| stdout: outputStream, | |
| glob: { | |
| // Disable glob expansion. Pass args like '--filter=*' as-is. | |
| isGlobPattern: () => false, | |
| match: async () => [], | |
| }, | |
| }), | |
| setTimeout(cmd.timeout ?? 30 * 1000), | |
| ]); | |
| await outputStream.close(); | |
| outputStream = null; | |
| let output = readFileSync(outputStreamPath, 'utf-8'); | |
| let commandLine = `> ${cmd.command}`; | |
| if (exitCode !== 0) { | |
| commandLine = (exitCode === undefined ? '[timeout]' : `[${exitCode}]`) + commandLine; | |
| } else { | |
| // only allow ignore output if the command is successful | |
| if (cmd.ignoreOutput) { | |
| output = ''; | |
| } | |
| } | |
| newSnap.push(commandLine); | |
| if (output.length > 0) { | |
| newSnap.push(replaceUnstableOutput(output, caseTmpDir)); | |
| } | |
| if (exitCode === undefined) { | |
| break; // Stop executing further commands on timeout | |
| } | |
| } finally { | |
| if (outputStream !== null) { | |
| try { | |
| await outputStream.close(); | |
| } catch { | |
| // Ignore errors during cleanup close | |
| } | |
| } | |
| } |
| /** | ||
| * The timeout in milliseconds for the command. | ||
| * If not specified, the default timeout is 30 seconds. | ||
| */ | ||
| timeout?: number; | ||
| } | ||
|
|
||
| interface Steps { | ||
| ignoredPlatforms?: string[]; | ||
| env: Record<string, string>; | ||
| commands: (string | Command)[]; | ||
| /** | ||
| * Commands to run after the test completes, regardless of success or failure. | ||
| * Useful for cleanup tasks like killing background processes. | ||
| * These commands are not included in the snap output. | ||
| */ | ||
| after?: string[]; |
There was a problem hiding this comment.
The PR description focuses on reloading config on watch restart, but this change also expands the snap-test JSON schema (Command.timeout, Steps.after) and adds new test fixtures. If this broader scope is intentional, it would help to either mention it in the PR description or split the test-runner/schema changes into a separate PR for easier review and rollout.
Bumps the vite-task git-dependency crates (`fspy`, `pty_terminal_test`, `pty_terminal_test_client`, `snapshot_test`, `vite_path`, `vite_powershell`, `vite_str`, `vite_task`, `vite_workspace`) from `4003f65` to `85d4e73`. ## Changelog All three new entries are internal bug fixes with no user-facing CLI/config changes, so no docs updates are needed: - Task cache now supports much larger automatically tracked input sets without hitting wincode's default 4 MiB sequence preallocation limit ([#554](voidzero-dev/vite-task#554)) - npm workspace patterns beginning with `./` now discover matching packages correctly ([#547](voidzero-dev/vite-task#547)) - Failures while forwarding output from a started task process no longer incorrectly report that the process failed to spawn ([#506](voidzero-dev/vite-task#506)) Full diff: ``https://github.com/voidzero-dev/vite-task/compare/4003f65a3e5e3d957ff81b157e85e6ee41cc59fc...85d4e734c64c96c2ce60e734b3e202b5add53696#diff-06572a96a58dc510037d5efa622f9bec8519bc1beab13c9f251e97e657a9d4ed`` ## Verification - `cargo check --workspace` — clean (no breaking changes; the PTY snapshot runner compiles against the new vite-task crates without changes) - `cargo test` across the vite-plus crates (`vite_command`, `vite_error`, `vite_install`, `vite_js_runtime`, `vite_migration`, `vite_shared`, `vite_static_config`, `vite-plus-cli`, `vite_global_cli`) — all pass - PTY snapshot suite (global flavor): the task-runner/cache output cases that the bump could affect (`command_run_help`, `command_cache_*`, `shim_recursive_npm_run`) pass unchanged. The remaining failures in this sandbox are environmental (missing bootstrapped JS CLI `dist/bin.js`, and npm-registry TLS blocked by the proxy) and are left to CI, which bootstraps the CLI and registry bridge. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- _Generated by [Claude Code](https://claude.ai/code/session_01LGJPQK1jAPZVtj4eCuWMPm)_ Co-authored-by: Claude <noreply@anthropic.com>

Pass the build function as the restart callback to
buildWithConfigsinstead of an empty function, ensuring config is re-resolved when
watch mode detects config file changes.
Ref: rolldown/tsdown@1756b03
close VP-160