Repository navigation
fix: follow a replaced inputFileSystem when purging in watch - #21643
bjohansebas wants to merge 3 commits into
Conversation
NodeWatchFileSystem captures the input filesystem it is constructed with and purges that one on every change. Assigning `compiler.inputFileSystem` afterwards — as MultiCompiler's setter does for every child, and as virtual-filesystem plugins do — left the watcher purging an orphaned cache, so watch kept rebuilding from stale sources indefinitely. Re-point the watcher at the compiler's current filesystem on watchRun, and only while the watcher is still the one this plugin installed.
🦋 Changeset detectedLatest commit: 2bdf51a 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 |
|
This PR is packaged and the instant preview is available (2bdf51a). Install it locally:
npm i -D webpack@https://pkg.pr.new/webpack@2bdf51a
yarn add -D webpack@https://pkg.pr.new/webpack@2bdf51a
pnpm add -D webpack@https://pkg.pr.new/webpack@2bdf51a |
📝 WalkthroughWalkthrough
ChangesWatch input filesystem lifecycle
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #21643 +/- ##
==========================================
+ Coverage 94.23% 94.39% +0.16%
==========================================
Files 628 628
Lines 79952 80621 +669
Branches 23676 23812 +136
==========================================
+ Hits 75344 76106 +762
+ Misses 4608 4515 -93
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Generated code sizeComparing
675 asset(s) changed size, biggest 20 by raw change
7 runtime(s) changed which runtime modules they carry
Built |
The guard skipping the update when `watchFileSystem` was replaced never did anything: the tap only ever writes to the instance this plugin holds, so a watcher the user installed is out of reach either way. Drop it, and cover the contract against the rewrite that would break it — assigning through `compiler.watchFileSystem` instead. `fs.rmSync` needs Node.js 14.14 and the matrix goes down to 10.x, so the fixture teardown uses unlink + rmdir.
Merging this PR will regress 1 benchmark
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Memory | benchmark "many-modules-esm", scenario '{"name":"mode-development","mode":"development"}' |
821.1 KB | 2,303.2 KB | -64.35% |
| ⚡ | Memory | benchmark "devtool-eval", scenario '{"name":"mode-development-rebuild","mode":"development","watch":true}' |
949.8 KB | 476.9 KB | +99.18% |
| ⚡ | Memory | benchmark "many-modules-commonjs", scenario '{"name":"mode-production","mode":"production"}' |
10.7 MB | 8.9 MB | +21.27% |
| ⚡ | Memory | benchmark "css-modules", scenario '{"name":"mode-development-rebuild","mode":"development","watch":true}' |
917.6 KB | 761.4 KB | +20.51% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix/watch-purge-replaced-input-filesystem (2bdf51a) with main (5c5d009)
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c1a40c26-1f10-4888-bf86-2b477b26a640
📒 Files selected for processing (2)
lib/node/NodeEnvironmentPlugin.jstest/WatchInputFileSystem.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/node/NodeEnvironmentPlugin.js
| const watching = compiler.watch(watchOptions, (err) => { | ||
| if (finished) return; | ||
| finished = true; | ||
| if (err) return done(err); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target test ---'
cat -n test/WatchInputFileSystem.test.js | sed -n '180,255p'
printf '%s\n' '--- watcher implementation references ---'
rg -n -C 4 'class NodeWatchFileSystem|watch\(|close\(' lib/node/NodeWatchFileSystem.js test/WatchInputFileSystem.test.jsRepository: webpack/webpack
Length of output: 7242
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- test setup and watch-error controls ---'
cat -n test/WatchInputFileSystem.test.js | sed -n '1,180p'
rg -n -C 5 'watchFileSystem|watch\([^;]*err|callback\(.*err|watch\(\{' lib test | head -n 240
printf '%s\n' '--- compiler watch implementation ---'
rg -n -C 8 'watch\(watchOptions|watch\(watchOptions|class Watching|watchFileSystem\.watch|callback\(err' lib/Compiler.js lib/Watching.js libRepository: webpack/webpack
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Compiler.watch ---'
cat -n lib/Compiler.js | sed -n '536,558p'
printf '%s\n' '--- Watching lifecycle and watch error handling ---'
cat -n lib/Watching.js | sed -n '28,190p'
cat -n lib/Watching.js | sed -n '190,360p'
cat -n lib/Watching.js | sed -n '360,520p'
printf '%s\n' '--- NodeWatchFileSystem watch return and close contract ---'
cat -n lib/node/NodeWatchFileSystem.js | sed -n '30,170p'Repository: webpack/webpack
Length of output: 23644
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- Watching.close implementation ---'
cat -n lib/Watching.js | sed -n '477,590p'
printf '%s\n' '--- tests that force watch or watchRun errors ---'
rg -n -C 8 'watchRun|watchFileSystem\s*=|new NodeWatchFileSystem|done\(err\).*close|close\(\(\).*done' test | head -n 260
printf '%s\n' '--- watch callback type and all NodeWatchFileSystem callback error paths ---'
rg -n -C 5 'WatchMethod|callback\(err|callback\(null' lib/util/fs.d.ts lib/util/fs.js lib/node/NodeWatchFileSystem.jsRepository: webpack/webpack
Length of output: 23604
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
test = Path("test/WatchInputFileSystem.test.js").read_text()
watching = Path("lib/Watching.js").read_text()
node_watch = Path("lib/node/NodeWatchFileSystem.js").read_text()
checks = {
"test currently completes the error branch directly": (
"if (err) return done(err);" in test
),
"Watching forwards watch errors to the handler": (
"if (err) {" in watching
and "return this.handler(err);" in watching
),
"Watching.close closes the active watcher before its callback": (
"if (this.watcher) {" in watching
and "this.watcher.close();" in watching
and "this._closeCallbacks = [];" in watching
),
"NodeWatchFileSystem.close closes the underlying watcher": (
"this.watcher.close();" in node_watch
and "this.watcher = null;" in node_watch
),
}
for name, passed in checks.items():
print(f"{'PASS' if passed else 'FAIL'}: {name}")
assert all(checks.values())
print("Conclusion: an error delivered to the test callback can be followed by Watching.close(), and close performs watcher cleanup before invoking its close callback.")
PYRepository: webpack/webpack
Length of output: 551
Close the watcher before reporting err. When the callback receives an error, call watching.close(() => done(err)) so watcher cleanup completes before Jest finishes.
|
Please fix lint |
A MultiCompiler reports a cycle per child that rebuilt, so asserting on the second call to the handler read one child's bundle before the other had rebuilt — which is what the Bun runtime hit. Wait for every bundle to carry the change instead, bounded by a deadline that names the failure, and type the watching a concurrent call can leave undefined.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/WatchInputFileSystem.test.js`:
- Around line 155-158: Adjust the timeout handling in the watch test around the
deadline timer and its surrounding test so the Jest test-specific timeout
exceeds the full initial-build, watcher-close, and 15-second rebuild-wait
duration, or reduce the deadline to fit within the configured 30-second limit.
Keep the existing failure behavior in the deadline callback unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cd10bd1c-f0d6-41a0-871a-2bb01188ea20
📒 Files selected for processing (1)
test/WatchInputFileSystem.test.js
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
webpack/schema-utils(auto-detected)webpack/tapable(auto-detected)
| if (err2) return finish(err2); | ||
| deadline = setTimeout(() => { | ||
| finish(new Error("the rebuild never picked the change up")); | ||
| }, 15000); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline test/WatchInputFileSystem.test.js --items all --type function
rg -n -C 6 \
'jest\.setTimeout|testTimeout|15000|setTimeout' \
test/WatchInputFileSystem.test.js package.json .Repository: webpack/webpack
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- test file timeout-related lines ---'
rg -n -C 8 'jest\.setTimeout|testTimeout|15000|setTimeout|after(All|Each)|watcher\.close|close\(' test/WatchInputFileSystem.test.js
printf '%s\n' '--- repository Jest configuration ---'
rg -n -C 5 'testTimeout|setTimeout|jest\.setTimeout|slowTestThreshold|testMatch|testRegex' \
package.json \
jest.config.* \
config \
test 2>/dev/null | head -n 300
printf '%s\n' '--- relevant test block ---'
sed -n '110,190p' test/WatchInputFileSystem.test.jsRepository: webpack/webpack
Length of output: 21677
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- complete test setup and affected tests ---'
sed -n '1,270p' test/WatchInputFileSystem.test.js
printf '%s\n' '--- watcher close implementation and related tests ---'
rg -n -C 8 'class .*Watcher|close\(callback|close\(.*callback|watcher.*close|Watching.*close' lib test | head -n 400
printf '%s\n' '--- Jest timeout semantics in repository configuration ---'
sed -n '1,45p' jest.config.js
rg -n -C 4 'WatchInputFileSystem|testTimeout|jest\.setTimeout' test/WatchInputFileSystem.test.js test/setupTestFramework.js jest.config.js package.jsonRepository: webpack/webpack
Length of output: 29681
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '477,565p' lib/Watching.js
sed -n '60,90p' lib/MultiWatching.js
sed -n '1465,1505p' lib/Compiler.jsRepository: webpack/webpack
Length of output: 3857
Give this test an explicit timeout margin.
jest.config.js sets a 30-second timeout, but the 15-second deadline starts after the initial build and watcher setup. Watching.close() can also wait for an active build before calling done. Set a test-specific timeout above the full operation time, or shorten the deadline.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/WatchInputFileSystem.test.js` around lines 155 - 158, Adjust the timeout
handling in the watch test around the deadline timer and its surrounding test so
the Jest test-specific timeout exceeds the full initial-build, watcher-close,
and 15-second rebuild-wait duration, or reduce the deadline to fit within the
configured 30-second limit. Keep the existing failure behavior in the deadline
callback unchanged.
Source: MCP tools
alexander-akait
left a comment
There was a problem hiding this comment.
It is a breaking change
Abhirup0
left a comment
There was a problem hiding this comment.
The issue being addressed is real—rebuilding against an orphaned CachedInputFileSystem after reassigning compiler.inputFileSystem causes watch rebuilds to serve stale files in multi-compiler and custom dev-server setups.
However, as @alexander-akait noted, modifying this in NodeEnvironmentPlugin across all setups is a breaking behavior change:
- Lifecycle Inconsistency with
beforeRun:NodeEnvironmentPluginexplicitly guardsbeforeRunandshutdownwithif (compiler.inputFileSystem === inputFileSystem). This was designed so custom filesystems aren't purged by Webpack's default node plugin. ReassigningwatchFileSystem.inputFileSysteminwatchRunbreaks that assumption and forces all custom filesystems intoNodeWatchFileSystem'spurge(item, { exact })calls. - Wrapper Decoupling: Updating the closed-over
watchFileSysteminstance inside thewatchRunhook ignores whethercompiler.watchFileSystemhas been replaced or wrapped by other plugins.
To move this forward, we should clarify the intended ownership: if Webpack core is to manage cache purging for user-supplied inputFileSystems in watch mode, should this also apply to beforeRun, or should MultiCompiler.prototype.inputFileSystem / the caller be responsible for synchronizing the watcher reference?
Summary
NodeWatchFileSystemcaptures the input filesystem it is constructed with and purges that one on every change, so assigningcompiler.inputFileSystemafterwards leaves the watcher purging an orphaned cache and watch keeps rebuilding from stale sources indefinitely.MultiCompiler's owninputFileSystemsetter hands one filesystem to every child, which is exactly this shape.Nobody is broken today, because the projects that hit this already worked around it — and the workarounds are the argument for fixing it. Next.js shares one input filesystem across its dev-server
MultiCompilerchildren, then unregistersNodeEnvironmentPlugin'sbeforeRuntap viahooks.beforeRun.interceptand purges the whole cache on everydone(hot-reloader-webpack.ts). morjs'ReplaceInputFileSystemPlugindeep-importswebpack/lib/node/NodeWatchFileSystemto rebuild the watcher by hand (webpack.ts). Both become unnecessary, and Next.js could purge incrementally instead of discarding its cache each build.This also unblocks sharing one
CachedInputFileSystemacrossMultiCompilerchildren in core: on 3000 modules with 4 configs it takesstat13616 -> 3404,readFileandreadlink12808 -> 3202, and the retained cache 20.3 MB -> 5.1 MB. Before this fix that sharing produced stale bundles.What kind of change does this PR introduce?
fix
Did you add tests for your changes?
Yes —
test/WatchInputFileSystem.test.jscovers a replacedinputFileSystemon a singleCompilerand on every child of aMultiCompiler, both of which fail without the fix, plus one pinning that a watcher the user installed is never re-pointed.Does this PR introduce a breaking change?
No.
NodeEnvironmentPluginonly ever writes to the watcher it installed itself, so awatchFileSystemsupplied or wrapped by the user is unaffected.If relevant, what needs to be documented once your changes are merged or what have you already documented?
n/a
Use of AI
Yes. Claude Code found the bug while measuring input-filesystem duplication across
MultiCompilerchildren, reduced it to a minimal reproduction, wrote the fix and the tests, and researched the affected ecosystem usages cited above. Every claim here was reproduced locally or read from the linked source.Summary by CodeRabbit
Bug Fixes
Tests