Conversation
nodejs-github-bot
added
the
lib / src
lib/_http_client.js
Outdated
There was a problem hiding this comment.
If we use .bind() in any of these cleanup PRs, we won't be able to/shouldn't backport to v6.x or v4.x.
There was a problem hiding this comment.
Yep, I know. I'm fine with these not being backported or only being partially backported.
lib/http.js
Outdated
There was a problem hiding this comment.
nit: Can we replace var with const?
|
Ping @nodejs/http @nodejs/collaborators I will rebase this early next week and address the feedback already provided but I would appreciate a few more eyes on it |
cjihrig
left a comment
cjihrig
left a comment
There was a problem hiding this comment.
LGTM if you drop the use of bind(), just for the sake of backporting.
|
How about I do a separate backport pr without bind? |
|
That works. |
mcollina
left a comment
mcollina
left a comment
There was a problem hiding this comment.
LGTM, but it does not currently apply cleanly to master.
Some of those changes would make it harder to backport future changes from 8 to 6. I think the files read better this way, so I'm 👍 .
|
Yeah I'll be rebasing later on today and doing another CI run. |
|
New CI after rebase: https://ci.nodejs.org/job/node-test-pull-request/6941/ |
| self._last = true; | ||
| self.shouldKeepAlive = false; | ||
|
|
||
| const oncreate = (err, socket) => { |
There was a problem hiding this comment.
Why this change from function declaration to variable declaration?
There was a problem hiding this comment.
The function was using a closure around self. I modified it to use lexical this with the arrow function.
PR-URL: #11594 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: #11594 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
|
Landed in 5425e0d...74c1e02 |
PR-URL: nodejs#11594 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#11594 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
|
This will need to be manually backported to v7.x |
9 participants
Some simple code hygiene cleanups.
module.exports = {}pattern,selfChecklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
http