feat(build): watch publicDir and sync changes to outDir in watch mode#22667
feat(build): watch publicDir and sync changes to outDir in watch mode#22667Vladexy88x wants to merge 1 commit into
Conversation
When running `vite build --watch`, the public directory was only copied once at startup. Watch publicDir with chokidar and sync added, changed and deleted files directly into the resolved outDirs, without triggering a full rebuild since public assets bypass the bundler. Uses awaitWriteFinish so files truncated by an in-progress write are not copied half-written, and logs sync errors instead of crashing the watch process. fixes vitejs#18655 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
daltino
left a comment
There was a problem hiding this comment.
Review: feat(build): watch publicDir and sync changes to outDir in watch mode
This is a great quality-of-life fix for a real pain point in --watch mode! The intent is clear and the approach is sensible. I have a few suggestions before this lands.
✅ What looks good
- Correct guard:
publicDir && options.copyPublicDir && fs.existsSync(publicDir)mirrors the existing copy-on-start logic nicely. - Lazy-importing
chokidarwithawait import('chokidar')is consistent with how the rest of the watch code works. - Scoping the watcher to
publicDironly keeps the blast radius small.
🟡 Issues & Suggestions
1. Watcher is never closed (potential resource leak)
The most important issue in the visible patch. The watcher instance returned by chokidar.watch() should be closed when the Rollup watcher closes, otherwise the process can hang on Ctrl+C and file descriptors leak.
// after creating the chokidar watcher
watcher.on('error', (err) => config.logger.error(`[public watcher] ${err}`))
// close it when the rollup watcher is closed
rollupWatcherObj.on('close', () => {
chokidarWatcher.close()
})(Naming it chokidarWatcher or similar avoids shadowing the outer watcher variable that Vite already uses.)
2. unlink / unlinkDir events are likely missing
The description mentions add/change copy events, but deleting a file from publicDir during --watch should also remove it from outDirs. Without handling unlink and unlinkDir, the out dir silently diverges from the source.
chokidarWatcher.on('unlink', (file) => {
const relative = path.relative(publicDir, file)
for (const outDir of outDirsArray) {
fs.rm(path.join(outDir, relative), { force: true }, () => {})
}
})3. outDirsArray might include paths inside publicDir
If someone accidentally configures outDir to be a subdirectory of publicDir, the watcher will pick up its own writes and loop. It's worth adding the same guard Vite uses elsewhere:
const safeOutDirs = outDirsArray.filter(
(outDir) => !outDir.startsWith(publicDir + path.sep),
)4. Symlinks and atomic saves
Chokidar's default config uses usePolling: false and doesn't follow symlinks. That's usually fine, but it's worth confirming the options passed here match what the main dev-server watcher uses (config.server.watch) so behaviour is consistent across modes. At minimum, document the deliberate choice.
5. No tests added
There are existing build --watch integration tests under playground/. A test that:
- Starts a watch build,
- Writes a new file to
public/, - Asserts it appears in
outDirwithout modifying any source file
…would prevent regressions and is the main thing missing before this is merge-ready.
6. Minor: prefer fs.promises.copyFile for consistency
The rest of build.ts uses async FS operations. If the copy inside the handler uses the synchronous API, it can block the event loop during large asset copies. Using fs.promises.copyFile (with a .catch(logger.error)) keeps things non-blocking.
Summary
| Resource leak (watcher not closed) | 🔴 blocker |
Missing unlink/unlinkDir handling |
🟠 should fix |
outDir ⊂ publicDir loop risk |
🟡 edge case |
| No tests | 🟠 should fix |
| Async copy | 🟢 nice-to-have |
Great start — with the watcher cleanup and unlink handling addressed this will be a solid addition!
Thanks for the thorough review @daltino, appreciate you digging in! 🙏 A few of these are actually already handled in the diff — I think the "visible patch" you reviewed may have been truncated, so let me point at the relevant lines: #1 — watcher close (blocker): Handled. The chokidar watcher is closed on the rollup watcher's close event: #2 — unlink / unlinkDir: Both are wired up. unlink routes through syncFile(file, true) which does fs.rmSync(dest, { force: true }), and unlinkDir removes the directory recursively: #4 — chokidar options consistency: The watcher spreads ...resolvedChokidarOptions — the exact options the dev server resolves from config.server.watch — so behaviour is consistent across modes. awaitWriteFinish is layered on top specifically to handle the atomic/truncate-then-write case (covered in the PR description), and ignoreInitial: true from those options avoids a duplicate startup copy. On the points that are genuinely open: #3 — outDir inside publicDir: Fair edge case. When emptyOutDir is on, the out dirs are already in the ignore list via resolvedChokidarOptions, so the loop can't happen there — but you're right that with emptyOutDir: false and an outDir nested under publicDir it could re-sync its own writes. The copy is idempotent so it wouldn't spin forever, but I'm happy to add an explicit filter to be safe. Will do. #5 — tests: Agreed this is the main gap. I called it out in the description: reliably exercising the mid-write race needs a deliberate truncate-then-delayed-write, which is timing-sensitive. A simpler playground test (write to public/, assert it appears in outDir without touching source) is straightforward and regression-proofs the common path — I'll add that. The race-specific test I'd still like maintainer input on. #6 — async copy: The sync API here is deliberate — a throw inside a chokidar listener becomes an uncaught exception, so everything's wrapped in try/catch and logged. I can switch to fs.promises.copyFile().catch(...) to avoid blocking on large assets; will weigh that against keeping the error handling simple. Thanks again! |
Description
When running
vite build --watch, thepublicdirectory is only copied once when the build starts. Any later change to a file inpublic/is never reflected in the out dir until a source file happens to trigger a rebuild.This PR watches
publicDirwith chokidar during build watch mode and syncs changes directly into the resolved out dirs:add/changeevents copy the file into every resolved out dirunlink/unlinkDirevents remove the corresponding file/directorypublic/straight from disk)fixes #18655
Implementation notes
resolvedChokidarOptions(same options the dev server passes to chokidar), soignoreInitial: trueprevents a duplicate startup copy and the out dirs are ignored whenemptyOutDiris on. It is gated on the same conditions as the initial copy inprepareOutDirPlugin(publicDirset,build.copyPublicDir, dir exists), and is closed via the rolldown watcher'scloseevent.awaitWriteFinish: { stabilityThreshold: 100, pollInterval: 10 }is set (user-overridable viabuild.watch.chokidar). Without it, a file written with a truncate-then-write pattern (e.g. anothervite build --watchprocess emitting a service worker intopublic/, which is exactly the reporter's setup) gets copied half-written at the moment of truncation, and chokidar's internal per-file event throttling drops the follow-up event, so the out dir keeps a 0-byte file indefinitely. Reproduced on both Windows (NTFS) and Linux (ext4/inotify) with truncate→write gaps of 1–20 ms.Testing
Verified manually against the built package on Windows and the chokidar layer on Linux:
public/sync todist/without triggering a rebuilddist/(the 1–20 ms cases fail withoutawaitWriteFinish)No automated test is included yet: reliably exercising the mid-write race requires a deliberate truncate-then-delayed-write pattern (a plain
writeFileSyncof a small file doesn't hit it), and I'd like maintainer input on whether a playground build-watch test with that pattern is acceptable, since it is timing-sensitive. Happy to add one.🤖 Generated with Claude Code