fs: replace a bind() with a top-level function#13474
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added the fs mcollina commented Jun 5, 2017
cc @mscdex |
refack left a comment
refack left a comment There was a problem hiding this comment.
So it's not a "useless" bind (it was a Currying null). I'd say you just replaced it with something better.
lib/fs.js Outdated
There was a problem hiding this comment.
Maybe we can make it more evident by adding an fd parameter here? Plus I'm sure V8 would prefer it.
lib/fs.js Outdated
There was a problem hiding this comment.
yes, that's the exact meaning. this will be the stream object. I'll add a comment to note that.
There was a problem hiding this comment.
EventEmitter instances always call their event handlers within the context of the EventEmitter. I'm not sure we need to explicitly document that in this one particular place when we do this same thing throughout core.
There was a problem hiding this comment.
I agree with @mscdex. I looked at the code too quickly earlier.
mcollina commented Jun 6, 2017
mscdex commented Jun 6, 2017
LGTM |
mcollina commented Jun 7, 2017
@refack I've amended the commit message, check if it is ok and I'll merge. |
mcollina changed the title 
nodejs#11225 introduce an unnecessary bind() when closing a stream. This PR replaces that bind() with a top-level function. PR-URL: nodejs#13474 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Evan Lucas <[email protected]>
mcollina commented Jun 7, 2017
Landed as 07ca288. |
nodejs#11225 introduce an unnecessary bind() when closing a stream. This PR replaces that bind() with a top-level function. PR-URL: nodejs#13474 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Evan Lucas <[email protected]>
#11225 introduce an unnecessary bind() when closing a stream. This PR replaces that bind() with a top-level function. PR-URL: #13474 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Brian White <[email protected]> Reviewed-By: Evan Lucas <[email protected]>
MylesBorins commented Jul 17, 2017
This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace the backport request label with do-not-land if it shouldn't land |
9 participants
#11225 introduce an unnecessary
bind() when closing a stream. This PR replaces that bind() with a
top-level function.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
fs
edit: updated according to @refack suggestion.