feat(hot): ask less of a custom transport - #2427
Conversation
Six required methods, of which three were only required because of how `createHot` happened to call them. Two of those are not a transport's job at all: `handler` is meaningless unless the transport is served over HTTP. The built-in WebSocket one carried it only to answer 426 to a plain request, which is the right answer for any transport that does not serve requests — so that is the default now, and the WebSocket stream no longer has a `handler` of its own. `hasClients` exists to skip building a payload nobody will read. That is an optimization a transport can make inside `publish`, and both built-ins already do. A transport that does not offer it is asked to publish rather than assumed to have no clients. Both stay accepted, so a transport written against 8.3.0 keeps working, and a TODO marks them for removal in the next major. What is required is now `onConnect`, `publish`, `publishTo` and `close` — and the validation names that shorter set when something is missing. Left alone deliberately: the client side keeps its class-constructor shape. It is what `client.webSocketTransport` has always been, so every custom client written for webpack-dev-server works here untouched; symmetry with the factory is not worth breaking those. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
🦋 Changeset detectedLatest commit: 703ab0a 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 |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 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 (2)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 5 remain after this review. WalkthroughThe custom transport contract now requires Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The transport changes and overlay-test synchronization have no established issue requiring a fix before merge. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The smaller transport contract preserves existing custom handlers and retains a rejection response for plain HTTP requests without one. No introduced security issue was established, though custom deployment behavior is not fully covered. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 43266814-0063-4aaa-b086-f2eb4c2c4067
📒 Files selected for processing (6)
.changeset/simpler-transport-contract.mdREADME.mdsrc/hot.jssrc/servers/WebSocketServer.jstest/hot.test.jstypes/hot.d.ts
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
Two review findings, both regressions from making the two methods optional. Dropping them from the required list stopped them being type-checked at all, so `handler: true` passed startup and threw from the endpoint instead — exactly the late, far-from-the-option failure this validation exists to prevent. Present and not callable is now rejected by name, while absent stays fine. `(eventStream.handler || upgradeRequired)(req, res)` also called the handler unbound. A stream written as an object literal reaching for `this` worked before this change, so the call goes back to being a method call and the default is a separate branch. Every other call on the stream was already a method call, so only this one had lost its receiver. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
CI surfaced this on an unrelated PR: "Execution context was destroyed, most likely because of a navigation". The fixture accepts no updates, so every rebuild is a full reload, and the watcher can see one write twice — file timestamp granularity is enough. The second reload lands inside the `evaluate` that triggers the next error and takes its execution context with it. Nothing in the assertion is wrong; it just had no guarantee the page had stopped moving. `settle()` exists for this and is now awaited before the page is touched again. The test predates the transport work on this branch — it came in with #2370 — and is not reachable from anything that work changes, but it fails this branch's CI, so it is fixed here rather than re-run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
…load The red `Client` job, properly diagnosed this time. The failing test is "turns an error overlay into a warning overlay on partial recovery", not the one the previous commit hardened — I had inferred it from the line number in the stack, and for a puppeteer error jest points at the blank line between tests, so the name was the only reliable signal and I had grepped it away. The race: recovering from a build that failed cannot be applied hot, so the page reloads. `waitForFunction` survives that, because puppeteer re-installs it in the new document, but the `page.evaluate` that read the overlay afterwards does not — and dies with "Execution context was destroyed" when the reload lands between the two. Reproduced locally at roughly one run in ten. Waiting and reading are now one step, through a `waitForOverlayText` helper that returns the text the wait matched, with a retry for the narrower race where the navigation arrives after it resolved. The assertion still reads the text, so a missing "WARNING" fails as a comparison rather than a timeout. Twenty consecutive runs of the case, and no occurrence of the error. The `settle()` added to "resets the runtime slot on a clean build" in the previous commit stays: that test has the same reload hazard, so guarding it is right even though it was not what CI was failing on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2427 +/- ##
==========================================
+ Coverage 95.91% 95.93% +0.01%
==========================================
Files 18 18
Lines 2006 2015 +9
==========================================
+ Hits 1924 1933 +9
Misses 82 82 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Summary
A custom
hot.transporthad to implement six methods. Three of those were only required because of howcreateHothappened to call them, and two are not a transport's job at all:handleris meaningless unless the transport is served over HTTP. The built-in WebSocket one carried it only to answer426to a plain request — which is the right answer for any transport that does not serve requests, so it is now the default and the WebSocket stream has nohandlerof its own. The built-in demonstrates the smaller contract rather than merely permitting it.hasClientsexists to skip building a payload nobody will read. That is an optimization a transport can make insidepublish, and both built-ins already do. A transport that does not offer it is now asked to publish and decides for itself, rather than being treated as having no clients — which is what the old!eventStream.hasClients()would have done, by throwing.So the required set is
onConnect,publish,publishTo,close, and the validation names that shorter list when something is missing.handlerandhasClientsare still accepted, so a transport written against 8.3.0 keeps working untouched.A
TODObesideCLIENT_STREAM_METHODSmarks both for removal in the next major, with the reasoning, so whoever does that release finds it at the code rather than in an issue.On the client side — deliberately unchanged
The obvious next thought is to make the browser client symmetrical with this factory. I would not: the class-constructor shape is
client.webSocketTransport, which webpack-dev-server has shipped for years, so every custom client written against it works here as-is. Symmetry is not worth breaking those, and the reasoning is in the commit message so it does not get "fixed" later.What kind of change does this PR introduce?
feature — a relaxed requirement for transport authors
Did you add tests for your changes?
Yes, two:
hasClientsstill receives progress events, since that is the only place it was read.The existing
426case now also asserts the body, because it is what proves the fallback serves it rather than the WebSocket stream's own handler.Both fallbacks teeth-checked — removing them gives
eventStream.handler is not a functionandeventStream.hasClients is not a functionacross three cases.Verified locally: end-to-end 110/110, non-browser 6821 passed (the
hotsuite at 68), lint and both typechecks clean.test/logging.test.jsfails 74/74 on a cleanmaintoo, unrelated to this.types/hot.d.tsis regenerated withbuild:typesrather than hand-edited, sohandlerandhasClientsare?:there.Does this PR introduce a breaking change?
No. Nothing that worked before stops working: a transport implementing all six is unaffected, and the two methods only became optional. The observable behaviour of both built-in transports is unchanged — the WebSocket endpoint still answers
426on a plainGET, just from the default rather than from its own method.If relevant, what needs to be documented once your changes are merged or what have you already documented?
Documented here: the README's custom-transport section now shows four required methods and four optional ones, with what happens when each optional one is absent.
Use of AI
AI-assisted (Claude Code). Used to work out which of the six methods the middleware actually needs, implement the fallbacks, write the tests and the README, and verify: each fallback was teeth-checked by removing it and confirming the matching tests fail.
🤖 Generated with Claude Code
https://claude.ai/code/session_01UjuMAuk9o6UazjHzcAQCTA
Generated by Claude Code
Summary by CodeRabbit