fix: never watch the compiler output path for static changes - #5747
alexander-akait wants to merge 5 commits into
Conversation
A static directory frequently contains the compilation's own output, either because `output.path` sits inside it or because `writeToDisk` writes there. Every build then rewrites those files and the static watcher turns a single compilation into a full page reload, which the middleware had already delivered. Where a plugin copies that output back into the copied directory, the same watch reports every generation of the copy and the reloads never stop: measured on a watch with no source edits, 191 reloads in 15s, one per rebuild, from `public/build/build/build/build/main.js` and deeper. Exclude each compiler's output path from the static watcher, as Vite does for `build.outDir`. Paths named explicitly through `watchFiles` are left alone, since those are watched because the user asked for them.
🦋 Changeset detectedLatest commit: 6d37f78 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 3 remain after this review. Walkthrough
Priority: ⬇️ Low 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
lib/Server.jsESLint failed to execute (timeout). test/ports-map.jsESLint skipped: the matched ESLint configuration already failed (timeout). test/server/static-watch-output-path.test.jsESLint skipped: the matched ESLint configuration already failed (timeout). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: e7bb3e75-fb30-4b3a-b551-9444c39a29f1
📒 Files selected for processing (5)
.changeset/fix-static-watch-ignores-output-path.mdlib/Server.jstest/ports-map.jstest/server/static-watch-output-path.test.jstypes/lib/Server.d.ts
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
…s live A root output path already ends in the separator, so appending a second one produced a prefix like `//` that matched nothing below it and every descendant stayed watched. The test established readiness from `getWatched()`, which fills in during chokidar's initial scan rather than at the end of it, so a write could still be dropped and the absence of a reload proved nothing. Rewrite the control file until the watcher reports it instead: the first reload is evidence the scan is over. Reverting lib/Server.js to its base version now fails both assertions.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: bc6d2dd0-fd06-4b66-bb65-b624de973c06
📒 Files selected for processing (2)
lib/Server.jstest/server/static-watch-output-path.test.js
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
…ported Waiting for the root control file to report a change proves the watcher is live, but not that chokidar has discovered the nested file inside the output directory, so a write could still be lost and the zero-reload assertion could pass over a broken predicate. Assert instead on what the watcher decided to watch, which no longer depends on event timing at all, and rewrite the ignored file throughout the window rather than once so no single lost write can hide a regression. Reverting lib/Server.js to its base version fails all three assertions.
Matching a filesystem-root output path correctly turned out to be worse than matching it not at all. `output.path` defaults to `/` under an in-memory filesystem and is routinely left there — test/fixtures/client-config does exactly that — and `/` contains every static directory there is, so the watcher stopped reporting anything. CI caught it: "should work and log static changes" waited 120s for a reload that no longer came. Exclude an output path only where it is strictly inside the directory being watched, which is the case the change is for: a build emitting into a folder that is also served statically. An output path equal to or containing that folder is left alone, because the user pointed static at it deliberately. The matcher is now built per watched directory rather than once for the server, and the new regression test fails against the previous version.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Advanced
Run ID: 33ef48a4-637d-4ad8-bc4b-d8418e34c58e
📒 Files selected for processing (4)
lib/Server.jstest/ports-map.jstest/server/static-watch-output-path.test.jstypes/lib/Server.d.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- types/lib/Server.d.ts
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review.
`static.directory` is taken from the options as given, so it can be relative, while `outputPath` and the paths the watcher reports are absolute. Comparing them unresolved made the containment check false every time, and the exclusion quietly matched nothing: with a relative static directory the output directory was still watched. Resolve the static directory, each output path and the target, and decide containment with `path.relative` so a `..` segment cannot read as nested. Reverting lib/Server.js fails the new assertion.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5747 +/- ##
==========================================
+ Coverage 90.74% 90.84% +0.10%
==========================================
Files 14 14
Lines 6329 6400 +71
==========================================
+ Hits 5743 5814 +71
Misses 586 586 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Summary
A static directory very often contains the compilation's own output — either because
output.pathsits inside it (static/, with the bundle emitted tostatic/build) or becausewriteToDiskputs it there. Every build then rewrites those files, the static watcher sees the write, and a single compilation becomes a full page reload that the middleware had already delivered.Where a plugin copies that directory back into itself, the watch reports every generation of the copy and the reloads never stop. Measured with no source edits at all, on
output.pathinside the watched directory withwriteToDisk:The reload paths show the copies stacking up:
public/build/main.js,public/build/build/main.js,public/build/build/build/main.js, …It is not limited to copying. With no copy plugin at all, output inside a watched static directory produces one spurious reload per build (1 → 0 with this change).
This excludes each compiler's output path from the static watcher, which is what Vite does for
build.outDir— itsresolveChokidarOptionsaddsescapePath(outDir) + '/**'to chokidar'signored. Paths named explicitly throughwatchFilesare deliberately left alone, since those are watched because someone asked for them.Multi-compiler is handled: every compiler's
outputPathis excluded.What kind of change does this PR introduce?
fix
Did you add tests for your changes?
Yes,
test/server/static-watch-output-path.test.js. Reverting only thelib/Server.jschange fails both of its assertions. The test waits for chokidar's initial scan to complete before writing, because events raised before the watcher is ready are dropped and the assertion would otherwise pass without the fix.test/e2e/static-directory.test.js(19 tests),test/e2e/watch-files.test.jsandtest/e2e/hot-and-live-reload.test.js(61 tests together) all still pass.Does this PR introduce a breaking change?
No, with one behaviour note: if a static directory is the output directory, changes written there by something other than webpack no longer trigger a static reload. Compilation output already reaches the client through the middleware.
If relevant, what needs to be documented once your changes are merged or what have you already documented?
The
static.watchdocs could state that the compiler'soutput.pathis never watched, so emitting a bundle into a static directory is safe.Use of AI
AI-assisted (Claude Code). It was used to reproduce the reload loop, to write the fix and the test, and to measure the before/after reload counts quoted above. Every number came from a run, not an estimate, and all output was reviewed before committing.
Related: this is the watcher-layer half of the problem behind #4232. The copy-layer half is webpack/webpack#22216 — the two are independent, and each fixes a different symptom (reload storm vs. rebuild storm).
🤖 Generated with Claude Code
https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Tests