http: simple code refactoring#11594
Uh oh!
There was an error while loading. Please reload this page.
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
jasnell commented Mar 17, 2017
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.
jasnell commented Mar 17, 2017
How about I do a separate backport pr without bind? |
cjihrig commented Mar 17, 2017
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 👍 .
jasnell commented Mar 20, 2017
Yeah I'll be rebasing later on today and doing another CI run. |
jasnell commented Mar 20, 2017
New CI after rebase: https://ci.nodejs.org/job/node-test-pull-request/6941/ |
| self._last=true; | ||
| self.shouldKeepAlive=false; | ||
| constoncreate=(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 <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
PR-URL: #11594 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
jasnell commented Mar 20, 2017
Landed in 5425e0d...74c1e02 |
PR-URL: nodejs#11594 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
PR-URL: nodejs#11594 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
MylesBorins commented Mar 28, 2017
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