feat(node): expose client connection events through on and once - #4061
feat(node): expose client connection events through on and once#4061wangyusheng1985 wants to merge 5 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4061 +/- ##
============================================
- Coverage 86.38% 85.90% -0.49%
+ Complexity 1455 1431 -24
============================================
Files 1259 1248 -11
Lines 205513 193261 -12252
Branches 170714 158541 -12173
============================================
- Hits 177542 166021 -11521
+ Misses 23541 23168 -373
+ Partials 4430 4072 -358
🚀 New features to boost your workflow:
|
|
@T1B0 please check :) |
Signed-off-by: wangyusheng1985 <wangyusheng1985@users.noreply.github.com>
84d8577 to
a02da5d
Compare
|
thanks for this PR @wangyusheng1985 ! i think we should also include an 'end' event as well since most emitter api consumer rely on it for clean up purpose. Also could you please add an e2e test ? it would be nice to ensure we don't break this feature in the future. |
| }); | ||
| this.connection.on('error', (error: Error) => { | ||
| this._failQueue(error); | ||
| if (this.listenerCount('error') > 0) |
There was a problem hiding this comment.
any reason to check listeners presence only on error event ?
There was a problem hiding this comment.
Could you point to the specific listener-presence check you mean? The patch evidence describes typed on/once for connecting, connected, and error events and forwarding of transport lifecycle events, but it does not describe a listener check that applies only to the error event or the reasoning behind such a check.
There was a problem hiding this comment.
i just meant the listenerCount('error') > 0 check. Other events are just emited regardless of their listenerCount
There was a problem hiding this comment.
Thanks for the review. I’ll address this in the existing pull request and rerun the focused validation.
| this._resetSession(); | ||
| this.emit('eviction', error); | ||
| }); | ||
| this.connection.on('disconnected', () => { |
There was a problem hiding this comment.
should we publish "disconnect" and "eviction" events since it seem symetrical to "connected" event ?
There was a problem hiding this comment.
Thanks for the review. I’ll address this in the existing pull request and rerun the focused validation.
|
Checked: the requested event API and same-socket retry handling are implemented. The Node unit tests, lint, and build pass, and the independent review reported pass. |
|
Implemented the |
| deadline?: number | ||
| }; | ||
|
|
||
| export type ClientEventMap = { |
There was a problem hiding this comment.
The requested end event is missing from ClientEventMap and is never emitted. Neither a graceful disconnect nor client.destroy() notifies public listeners of termination. Please add the event and unit and end-to-end coverage.
There was a problem hiding this comment.
Thanks for the review. I’ll address this in the existing pull request and rerun the focused validation.
github-actions
Bot
added
S-waiting-on-author
4 participants
Why
The public Node.js
Clienthid theEventEmitterowned by its command stream, so applications could not observe connection lifecycle events with the standard Node.jsonandonceAPIs.What
Expose typed, chainable
onandoncemethods forconnecting,connected, anderror. Forward transport lifecycle events through the pooled raw client and announce every connection attempt while avoiding duplicate notifications for shared attempts. Preserve existing error handling when no user error listener is registered.Add focused coverage for initial connections, one-time listeners, transport errors, and retries on the same socket. Document event names, timing, and error behavior in the Node SDK README.
Closes #2396
Testing
node --import @swc-node/register/esm-register --test --test-name-pattern='emits connecting when an attempt retries on the same socket' src/client/client.connection.test.tsValidation observed for this change:
cd /workspace/repository/foreign/node && node --import @swc-node/register/esm-register --test --test-name-pattern='emits connecting when an attempt retries on the same socket' src/client/client.connection.test.tsnpm --prefix /workspace/repository/foreign/node run lint && npm --prefix /workspace/repository/foreign/node run buildnpm --prefix /workspace/repository/foreign/node run test:unitFixes #2396