Repository navigation
Conversation
…ames HMRModule.on() mapped "vite:X" to "bun:" + event.slice(4), which keeps the colon and registers the handler under "bun::X", so handlers added with the vite: alias never fired. off() did not map the name at all, so it could not remove a handler by its vite: name either. Both now go through one helper that strips the full "vite:" prefix.
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 6:05 AM PT - Aug 12th, 2026
❌ @robobun, your commit 09f2cc2 has some failures in 🧪 To try this PR locally: bunx bun-pr 37786That installs a local version of the PR into your bun-37786 --bun |
|
Status: reproduced on the current release with the two tests in this PR ( CI (build 92988): 179 of 181 jobs passed. The build is marked failed only because the two macOS 26 aarch64 test jobs expired without ever starting; that queue is backed up for every build right now (about 130 of these jobs queued, the oldest waiting over 5 hours, and a batch of other branches expired at the same minute), so a rebuild would most likely expire the same way and I am not retriggering. The annotated test failures are all retry-passed flakes in unrelated files (napi, sqlite, grpc-js, cron, next-pages, and so on); nothing in test/bake failed anywhere. The change is platform-independent TypeScript in the embedded HMR runtime and its tests passed on every other lane, so this is ready for review as is; the two missing lanes can be retried from Buildkite if wanted. |
There was a problem hiding this comment.
LGTM — clear off-by-one fix (.slice(4) on a 5-char prefix) plus the missing normalization in off(), factored into one shared helper.
What was reviewed
normalizeEventName:"vite:".lengthis 5, so"vite:beforeUpdate"now maps to"bun:beforeUpdate"instead of"bun::beforeUpdate"; non-vite:names pass through unchanged.on()reassignseventbefore registering the dispose closure, so auto-cleanup on module replace still finds the handler;off()on an already-normalized name is a no-op re-normalize.- Traced both new tests through
replaceModules(beforeUpdate → dispose → reload → afterUpdate) — the asserted message order matches, and the reworked test now fails on the old behavior instead of pinning it.
Extended reasoning...
Overview
Two-file change. src/runtime/bake/hmr-module.ts extracts a normalizeEventName() helper that rewrites vite:* event names to bun:* and routes both HMRModule.on() and HMRModule.off() through it. This fixes two bugs in one class: on() used .slice(4) on the 5-character "vite:" prefix (producing "bun::beforeUpdate", which nothing emits), and off() did not rewrite the name at all. test/bake/dev/hot.test.ts reworks the existing import.meta.hot on/off events test — which previously asserted the broken behavior — to assert that vite:* handlers actually fire, and adds a second test covering cross-prefix on/off.
Security risks
None. This is client-side dev-server HMR runtime code that maps one string prefix to another in an in-memory event handler table. No parsing of untrusted input, no filesystem/network/auth surface.
Level of scrutiny
Low-to-medium. The runtime change is a ~10-line refactor of an obviously-wrong string slice into a named helper, applied at both sibling sites (on and off) as REVIEW.md asks for. The behavior is documented in docs/bundler/hot-reloading.mdx and packages/bun-types/devserver.d.ts, so this is bringing the implementation in line with the stated contract rather than changing API surface. No CODEOWNERS entry covers src/runtime/bake/.
I traced the test expectations through replaceModules in hmr-module.ts: emitEvent("bun:beforeUpdate") runs before dispose, dispose removes the old module's on()-registered handlers via the closure (which captured the normalized name, so off() finds them), the new module body runs, then emitEvent("bun:afterUpdate") fires. The expected message sequences in both tests line up exactly with that order. The stale dispose callbacks left behind when a user calls off() manually before dispose are harmless — indexOf returns -1.
Other factors
The PR description states both new tests fail under USE_SYSTEM_BUN=1 and pass under bun bd test, and the rest of hot.test.ts still passes. The reworked test replaces one that was certifying the bug (it registered a vite:beforeUpdate handler and asserted it did not log), which is exactly the "read snapshot contents before committing" failure mode REVIEW.md warns about — good that it's fixed. The helper uses "vite:".length rather than a bare 5, which is the repo's preferred magic-number style.
Problem
import.meta.hot.on("vite:beforeUpdate", h)never fires; the same code with"bun:beforeUpdate"fires on every hot update. Docs and types sayvite:*is an alias ofbun:*.off()has the mirror problem: afteron("bun:beforeUpdate", h); off("vite:beforeUpdate", h),hstill fires.on()stripped four characters from the five charactervite:prefix, so the handler was stored underbun::beforeUpdate, a key nothing emits to.off()did not translate the name at all.vite:handler not to log, so it was pinning the bug.Fix
on()andoff()both pass the name through one helper that swaps the wholevite:prefix forbun:. Other names are stored unchanged.bun:*name, so a handler can be removed under either spelling, including the automatic removal of a replaced module's listeners. The emit side is untouched.vite:*handlers that were silently dead now run, with the same empty payload asbun:*handlers. Docs and types need no change.Background
import.meta.hotis the per-module HMR API page code sees, implemented in a runtime shipped to the browser.import.meta.hot.on/offsubscribe to events such asbun:beforeUpdateandbun:afterUpdate, emitted around each hot update.vite:*names are documented aliases so code written for Vite works unchanged.bun:*key, so a handler under any other key is unreachable.on()registers one that callsoff(), which is how a replaced module's listeners are dropped.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
Original description
What
import.meta.hot.on()andimport.meta.hot.off()in the dev server runtime now treat thevite:*event names as the aliases ofbun:*that the docs and types say they are.Repro
In a page served by the dev server:
Save the file. Nothing is logged, while the same code with
"bun:beforeUpdate"logs on every hot update.off()has the mirror image of the problem: afteron("bun:beforeUpdate", h); off("vite:beforeUpdate", h),hstill fires.Cause
HMRModule.on()insrc/runtime/bake/hmr-module.tsrewrote the name with"vite:"is five characters, so"vite:beforeUpdate"became"bun::beforeUpdate"(two colons) and the handler sat under a key thatemitEvent("bun:beforeUpdate")never looks at.off()did not rewrite the name at all, so it looked upeventHandlers["vite:..."], which never has anything in it.The existing test
import.meta.hot on/off eventsregistered avite:beforeUpdatehandler and expected it not to log, so it was pinning the broken behavior.Fix
Both
on()andoff()go through onenormalizeEventName()helper that replaces the wholevite:prefix withbun:. Because the same helper is used on both sides, a handler can be removed with either name, and the dispose callback thaton()registers (which removes a module's listeners when it is replaced) keeps working for handlers added under thevite:name. Names without the prefix are stored unchanged, as before.This is the behavior documented in
docs/bundler/hot-reloading.mdx("these events are also available with thevite:*prefix") and in the JSDoc foron()/off()inpackages/bun-types/devserver.d.ts, and it matches what the original code was clearly trying to do. Neither the docs nor the types need to change.The only user-visible change besides the alias working is that
vite:*handlers which were silently dead before now run, with the same (empty) payload thebun:*handlers get. A handler that throws is handled the same way a throwingbun:*handler already is: the client runtime logs it and falls back to a full reload (hmr-runtime-client.ts, the.catcharoundreplaceModules). Normalization happens only inon()/off();emitEvent/onEventare internal and typed to thebun:*names, so every key stored in the handler table is canonical and nothing else needs to know about the alias.Tests
test/bake/dev/hot.test.ts:import.meta.hot on/off eventsis reworked so that it asserts the events fire: abun:beforeUpdateand avite:beforeUpdatehandler both fire for the update that replaces the module, avite:afterUpdatehandler added by the new module fires when that update finishes, a handler that was passed tooff()never fires, and the replaced module's listeners (both prefixes) are gone on the next update.import.meta.hot.off accepts bun: and vite: event names interchangeablycoverson("bun:...")+off("vite:...")andon("vite:...")+off("bun:..."), with a third handler left registered to prove the event was emitted.Both tests fail on the current release (
USE_SYSTEM_BUN=1 bun test): the first only receivesbun:beforeUpdate (initial)andUpdated setup, the second receivesainstead ofstill listening. Both pass with this change (bun bd test), as does the rest ofhot.test.ts.