net: use object destructuring #20959
Conversation
lib/net.js
Outdated
| var port = options.port; | ||
| var localAddress = options.localAddress; | ||
| var localPort = options.localPort; | ||
| var { host = 'localhost', port, localAddress, localPort } = options; |
There was a problem hiding this comment.
This is not exactly the same as before. I think it is fine but host = 'localhost' !== options.host || 'localhost'. The first part is only going to set it to localhost in case host is undefined while the latter is going to do that for any falsy value.
There was a problem hiding this comment.
It's not strict equal but doesn't break CI. Should I revert it?
There was a problem hiding this comment.
I would prefer it if we reverted this change, or at least the part with localhost, other changes LGTM
| @@ -244,7 +243,7 @@ function Socket(options) { | |||
|
|
|||
| options.readable = options.readable || false; | |||
| options.writable = options.writable || false; | |||
| const allowHalfOpen = options.allowHalfOpen; | |||
| const { allowHalfOpen } = options; | |||
There was a problem hiding this comment.
Is there consensus that changes like this are desirable? It's not obvious to me that destructuring is better (or worse) than the existing straightforward assignment in this case.
There was a problem hiding this comment.
Umm... It's just personal preference. I'm free to revert it.
There was a problem hiding this comment.
@Trott I just guessed that you do not block this and it is author-ready. If that is not the case, please remove the label again.
There was a problem hiding this comment.
@BridgeAR Correct, not blocking. Thanks for checking.
starkwang
force-pushed
the
net-use-destructuring
branch
from
61c8535
to
c43fca8
Compare
BridgeAR
added
the
author ready
|
Landed in a69a29d |
PR-URL: #20959 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #20959 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]>
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes