Repository navigation
Conversation
replaceModules walks the importers of every module in a hot update payload with a per-module visited set, and pushed each self-accepting boundary with dispose callbacks onto a plain array. When one update carries two or more modules that reach the same boundary (two imports saved together, a JSON import, a plugin that emits several records per file), the boundary was queued twice. The first dispose pass ran its callbacks and set onDispose to null, the second threw "TypeError: mod.onDispose is not iterable" and the client fell back to a full page reload, losing import.meta.hot.data and page state. The dispose list is now a Set, like the reload list next to it. Fixes #29418
|
Status: ready for review. Reproduced on bun 1.4.3-canary (f42e980) with the new CI (build 112743): the new test and the rest of |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughChangesHMR disposal deduplication
Suggested reviewers: Priority: ➖ Normal — Impact reflects medium issue severity. Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to Batched HMR updates now dispose a shared accepting boundary once, preventing the dispose-callback error and preserving hot-update state. The targeted behavior is covered with no current merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — minimal, correct fix that mirrors the toReload Set right next to it.
What was reviewed:
replaceModulesin hmr-module.ts:for..ofiteration and.sizework identically on the Set;mod.onDispose = nullafter the first pass is exactly what made the array version throw on the duplicate entry.- Sibling collections:
toReloadis already a Set;toAcceptduplicates don't crash (callbacks aren't nulled) and are out of scope for this crash fix. - New test: uses the established
batchChanges()/expectMessageharness (event-driven, no sleeps), asserts strict order for both the batched and follow-up single-file update, and defers the state assertion to after eval 3 so it doesn't depend on the ordering fix in #31944.
Extended reasoning...
Overview
The PR changes toDispose in replaceModules (src/runtime/bake/hmr-module.ts) from HMRModule[] to Set<HMRModule>, with the corresponding .push → .add and .length → .size updates. The bug: the outer loop walks importers once per payload key with a per-key visited set, so a self-accepting boundary reached from two changed files was pushed onto toDispose twice; the dispose loop nulls mod.onDispose after running callbacks, so the second iteration threw TypeError: mod.onDispose is not iterable and forced a full page reload. toReload immediately above is already a Set for the same reason, so this brings toDispose in line. A new devTest in test/bake/dev/hot.test.ts batches two dependency writes into one hot update, asserts the boundary disposes exactly once, then verifies a subsequent single-file update still works and the final module state is correct.
Security risks
None. This is the client-side HMR runtime for the dev server; no auth, crypto, path handling, or untrusted-input parsing is involved. The only observable change is deduplicating which already-registered dispose callbacks run per hot update.
Level of scrutiny
Low-to-moderate. The source change is four lines and mechanical (array → Set), matching the pattern already used by toReload on the line above. Set iteration preserves insertion order and for..of works unchanged, so dispose ordering for the non-duplicate case is identical. I audited the sibling collections in the same function: toReload already dedupes; toAccept can in principle collect the same {cb, key} twice in diamond import graphs, but accept callbacks are not nulled after firing so there is no crash — that pre-existing behavior is reasonably left to the broader rewrite the PR references.
Other factors
The test is placed next to the existing import.meta.hot.dispose cleanup test in hot.test.ts, uses the harness's event-driven batchChanges() / expectMessage() (no sleeps), and covers both the batched multi-file case and a follow-up single-file update per the variant-matrix rule. The final toEqual on globalThis.state is deliberately taken after eval 3 so it does not depend on the separate reload-ordering fix. The PR description states the test fails on the unfixed build with the exact onDispose is not iterable error and that the rest of hot.test.ts/esm.test.ts still pass. No CODEOWNERS entries cover src/runtime/bake/ or test/bake/. Exit reason was dry_streak and there are no prior reviews or open threads on the timeline.
Problem
import.meta.hot.dispose(cb)breaks any hot update whose payload carries two or more modules that reach it. The HMR runtime throwsTypeError: mod.onDispose is not iterableinreplaceModules, logs it, and does a full page reload. Page state andimport.meta.hot.dataare lost. Triggers: two imports saved together, editing an imported JSON file (the payload carries the importer too), a plugin that emits several records per file (Bun dev server:K.onDispose is not iterableduring HMR when index.html links a stylesheet + entry has both accept() and dispose() #29418, svelte).replaceModules(src/runtime/bake/hmr-module.ts:622) walks importers once per payload key with a per-keyvisitedset, andtoDisposewas a plain array. A boundary reached from two keys was pushed twice. The first dispose pass ran the callbacks and setmod.onDispose = null, the second iteratednull.Fix
toDisposebecomes aSet<HMRModule>, liketoReloadnext to it. A boundary is disposed once per update no matter how many payload keys reach it. Which importers are walked and when a root triggers a reload is unchanged.test/bake/dev/hot.test.ts("import.meta.hot.dispose runs once when one update changes several imported files") fails on bun 1.4.3-canary and without thesrc/change (TypeError: mod.onDispose is not iterable, client exits), passes with it. All ofhot.test.tsandesm.test.tspass on the debug build.Background
replaceModulesapplies one hot update on the client. For each changed module it walks up the importer graph until it finds a boundary: a module that calledimport.meta.hot.accept()or usedimport.meta.hot.data. Boundaries are re-evaluated; theirdisposecallbacks run first.Fixes #29418
Notes
toDispose).import.meta.hot.on()registers its cleanup throughdispose(), so modules usingon()plusaccept()hit the same reload.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bake/dev/hot.test.ts