syntax es5 -> es6#16991
Conversation
syntax es5 -> es6
lib/_http_client.js
Outdated
|
|
||
| function validateHost(host, name) { | ||
| if (host != null && typeof host !== 'string') { | ||
| if (host !== null && typeof host !== 'string') { |
There was a problem hiding this comment.
Changes like this are not safe because it changes behavior, specifically undefined will no longer be considered in addition to null.
There was a problem hiding this comment.
thank you for your suggest!
|
Have you ran the relevant |
apapirovski
left a comment
apapirovski
left a comment
There was a problem hiding this comment.
Thanks for trying to improve Node @leeseean. I do have some concerns about this as it will make backporting harder going forward. Even if there are no performance differences for let/const for the current version of V8 (which I still think let has some downsides), there are definitely differences for older versions which means we won't be able to backport this PR and any PRs that subsequently change this code will need to be manually backported.
If this does go forward, I think this will need to be cleaned up. I'm seeing a lot of let where const would do.
lib/_http_client.js
Outdated
| if (method != null && !methodIsString) { | ||
| let method = options.method; | ||
| const methodIsString = (typeof method === 'string'); | ||
| if (method !== null && !methodIsString) { |
lib/_http_client.js
Outdated
| for (var i = 0; i < keys.length; i++) { | ||
| var key = keys[i]; | ||
| let keys = Object.keys(options.headers); | ||
| for (let i = 0; i < keys.length; i++) { |
There was a problem hiding this comment.
This won't pass our lint rules and will be worse for performance.
There was a problem hiding this comment.
Performance-wise V8 was supposed to have fixed this particular use case awhile ago, but you never know if there are weird edge cases or something yet. That is the main reason for my comment about checking for performance regressions just to be sure.
There was a problem hiding this comment.
I believe that @bmeurer flagged this as still being potentially problematic (in some limited scenarios) in an earlier PR that tried to get rid of the lint rule. Either way, this wouldn't pass the CI (linter) so it will need to be changed.
lib/_http_client.js
Outdated
| assert(parser && parser.socket === socket); | ||
|
|
||
| var ret = parser.execute(d); | ||
| let ret = parser.execute(d); |
There was a problem hiding this comment.
I'm not sure I see where this is changing. const?
There was a problem hiding this comment.
thank you for your suggest!
lib/_http_client.js
Outdated
| var port = options.port = options.port || defaultPort || 80; | ||
| var host = options.host = validateHost(options.hostname, 'hostname') || | ||
| let port = options.port = options.port || defaultPort || 80; | ||
| let host = options.host = validateHost(options.hostname, 'hostname') || |
There was a problem hiding this comment.
Do either of these change? const?
|
Closing this for #17011. |
5 participants
syntax es5 -> es6
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)