Expose and use once
#664
Expose and use once
#664
Conversation
| @@ -43,10 +43,11 @@ function Stripe(key, version) { | |||
| value: new EventEmitter(), | |||
| enumerable: false, | |||
| configurable: false, | |||
| writeable: false, | |||
| writable: false, | |||
| }); | |||
There was a problem hiding this comment.
heh, oops! Good catch.
| @@ -381,13 +381,16 @@ StripeResource.prototype = { | |||
| } | |||
| }); | |||
|
|
|||
| req.on('socket', (socket) => { | |||
| req.once('socket', (socket) => { | |||
There was a problem hiding this comment.
I'm not 100% sure that 'socket' event can only be sent once, and the body of the function seems to imply it might be more than once. Do you know of a guarantee that this is at-most-once?
There was a problem hiding this comment.
Look at the code - it checks a boolean flag socket.connecting (https://nodejs.org/api/net.html#net_socket_connecting) and in case of true waits for connection to be established by setting other event listener (socket.once).
In both cases this function are finishing with res.end() - so, I'm pretty sure it's designed to be running by once.
I can keep an on, as you wish, but see no error in this refactoring.
There was a problem hiding this comment.
@rattrayalex-stripe In fact, not, keeping on is an error. Just trace what will happen if this event handler will be called more than once - it will attempt to write to the req after calling req.end in previous call or in socket.once event handler.
There was a problem hiding this comment.
Okay great, I think that looks right. Thanks for digging here.
| @@ -43,10 +43,11 @@ function Stripe(key, version) { | |||
| value: new EventEmitter(), | |||
| enumerable: false, | |||
| configurable: false, | |||
| writeable: false, | |||
| writable: false, | |||
| }); | |||
There was a problem hiding this comment.
heh, oops! Good catch.
Fixes a misspelling in EventEmitter property declaration, exposes
oncemethod and usesoncewhere it's more appropriate thanon