Backport: repl: don't override all internal repl defaults #9774
Conversation
This commit adds a function to test/common.js that allows additional global variables to be whitelisted in a test. PR-URL: nodejs#7826 Reviewed-By: James M Snell <jasnell@gmail.com>
The createInternalRepl() module accepts an options object as an argument. However, if one is provided, it overrides all of the default options. This commit applies the options object to the defaults, only changing the values that are explicitly set. PR-URL: nodejs#7826 Reviewed-By: James M Snell <jasnell@gmail.com>
| @@ -19,11 +20,11 @@ function createRepl(env, opts, cb) { | |||
| cb = opts; | |||
| opts = null; | |||
| } | |||
| opts = opts || { | |||
| opts = util._extend({ | |||
There was a problem hiding this comment.
I'm curious why you wouldn't just use Object.assign here. Looking at the source of util._extend it looks to be identical. What am I missing?
There was a problem hiding this comment.
Object.assign() was shown to be slower in a few previous issues.
There was a problem hiding this comment.
If not in critical path, May be Object.assign() is ok enough.
There was a problem hiding this comment.
This is a backport, so I don't think it's a good idea to change things here.
There was a problem hiding this comment.
+1 If you have problem with implementation fix on master and it can come in another backport
There was a problem hiding this comment.
Thanks for the info. Just for the record, I asked out of curiosity - not because I expected it to change. I had no idea that Object.assign was slower. To see for myself, I ran some tests and also found that to be the case. Thanks for the clarification.
|
LGTM |
|
landed in 7b4268b...698bf2e |
This is a backport of #7826 to v4.
R= @thealphanerd