fix: return the socket from the client emitters and true from the server ones - #189
Conversation
|
Warning Review limit reached
Next review available in: 48 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesEmitter contracts and server API
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/decisions/0019-what-counts-as-a-breaking-change.md`:
- Around line 32-38: Reorder the “Raising engines.node, lowering it” row in the
decision table so it appears before “A correction toward measured real behaviour
with no observable change.” Preserve both rows’ existing classifications and
wording; only change their ordering so the Node version rule takes precedence
when both conditions match.
In `@src/connect-url.test.ts`:
- Around line 170-182: Update the connection test around connect() to await the
client’s next-tick connect_error event before leaving the try block and
restoring console.error. Preserve the existing emit assertions, and ensure the
failed connection’s scheduled console.error runs while the spy remains active.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ae3c5e64-257d-4493-afba-c6c3a78a38b5
📒 Files selected for processing (9)
docs/conformance.mddocs/decisions/0019-what-counts-as-a-breaking-change.mdscripts/conformance-report.mjssrc/connect-url.test.tssrc/contract.tssrc/emitter-returns.test.tssrc/index.test.tssrc/index.tssrc/mock-server.ts
Summary
contract.tsdeclared): voidon everyemitand every listener method, so smocket returnedundefinedwhere socket.io returns something.socket.on('a', f).on('b', g)threw. Found by installing the package and using it from outside, not by the suite, which never read a return value.Nothing here was guessed. Every position was read off a real socket.io 4.8.3 server first and the declaration written to match. The table is below, and it disagreed with the expectation in two places.
ClientSocketContractclient.emit(...)ServerSocketContractserverSocket.emit(...)true, not the socketServerContractio.emit(...)trueNamespaceContractnsp.emit(...)trueBroadcastContractio.to(r).emit(...)trueTimeoutEmitterContractclient.timeout(ms).emit(...)SocketTimeoutContractserverSocket.timeout(ms).emit(...)trueTimeoutBroadcastContractio.timeout(ms).to(r).emit(...)trueVolatileServerSocketserverSocket.volatile.emit(...)trueVolatileClientSocketclient.volatile.emit(...)ononceoffremoveAllListenersonAnyoffAnyonAnyOutgoingoffAnyOutgoingNamespaceContractnsp.on(...)ServerContractio.on(...)io.of('/'), aNamespaceThree things came out of that.
The emit shape is not uniform. Only the client returns the socket. Every server-side emit returns
true, including the server socket's, which was expected to match the client's and does not.TimeoutEmitterContracthad to split. It was shared, withSocketTimeoutContract extendsit, but the client's timed emit returns the socket and the server's returnstrue. An interface cannot narrow an inherited return type to an unrelated one, soSocketTimeoutContractdeclares its own two emit forms instead.ServerContract.onstaysvoid. It is the one position where socket.io disagrees with itself: the declaration saysthis, the runtime hands backio.of('/'). Narrowing toNamespaceContractfails theEnsure<>proof, since socket.io's declaredServerhas noname; narrowing tothiswould copy a promise its own runtime does not keep. Left alone and the reason is in the code.Related issue
No tracking issue. Depends on #188, which this branch is stacked on. Removes the section C entry #187 adds, which is why that section exists rather than the fix reading as a section A removal.
Checklist
src/emitter-returns.test.tsis 12 dual-run cases, one per row above plus a chaining case that proves two chained registrations both fire. Both targets are green at 188. One case failed on the mock first and was a bad assertion of mine, not a mock bug: it compared two separateclient.timeout()results, which are the same socket on real socket.io and a fresh wrapper on smocket. It now asserts chaining without asserting identity.Ensure<>proofs re-judge the narrowed returns, which is what caught the shared timeout interface.