-
Notifications
You must be signed in to change notification settings - Fork 30.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HTTP server adds .on
override to sockets but not .appendListener
#27199
Comments
Shouldn't this be used in this case? |
@SimonSchick this is probably really a proper way |
This looks to be an oversight/bug in the API to me, @nodejs/http ? |
A future proof solution would be to iterate on all Event emitter methods, and do the assignement for each method name that is equal to |
Off topic:
|
I don't think so. Mechanism of virtual functions is very nice in other languages but in JS it's a dirty and error-prone hack. Monkeypatching is acceptable for final objects but those in library are likely to be instantiated and extended. I'd vote for 'newlistener' listener |
Yes, sorry I didn't see the replies |
this bug caused by this code line Line 396 in 290faec
when I try srv.on('connection', (socket) => {
console.log(socket.on)
console.log(socket.addListener)
}) logged [Function: socketOnWrap]
[Function] |
Fixes: #27199 PR-URL: #27325 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
Here HTTP server adds a
.on
wrapper to a socket. But no other listener methods are wrapped so if we callsocket.addListener()
orsocket.prependListener()
we'll get other results than if we calledsocket.on()
.The text was updated successfully, but these errors were encountered: