Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: simple code refactoring #11594

Closed
wants to merge 2 commits into from
Closed

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Feb 28, 2017

Some simple code hygiene cleanups.

  • Use the more efficient module.exports = {} pattern,
  • Eliminate unnecessary uses of self
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

http

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Feb 28, 2017
@@ -283,7 +274,7 @@ ClientRequest.prototype._implicitHeader = function _implicitHeader() {

ClientRequest.prototype.abort = function abort() {
if (!this.aborted) {
process.nextTick(emitAbortNT, this);
process.nextTick(emitAbortNT.bind(this));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I know. I'm fine with these not being backported or only being partially backported.

exports.get = function get(options, cb) {
var req = exports.request(options, cb);
function get(options, cb) {
var req = request(options, cb);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can we replace var with const?

@jasnell
Copy link
Member Author

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

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if you drop the use of bind(), just for the sake of backporting.

@jasnell
Copy link
Member Author

jasnell commented Mar 17, 2017

How about I do a separate backport pr without bind?

@cjihrig
Copy link
Contributor

cjihrig commented Mar 17, 2017

That works.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

jasnell commented Mar 20, 2017

Yeah I'll be rebasing later on today and doing another CI run.
I will also be doing a backport PR to 6 to make it easier to backport future changes.

@jasnell
Copy link
Member Author

jasnell commented Mar 20, 2017

New CI after rebase: https://ci.nodejs.org/job/node-test-pull-request/6941/
will land after assuming it's green

self._last = true;
self.shouldKeepAlive = false;

const oncreate = (err, socket) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change from function declaration to variable declaration?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function was using a closure around self. I modified it to use lexical this with the arrow function.

jasnell added a commit that referenced this pull request Mar 20, 2017
PR-URL: #11594
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
jasnell added a commit that referenced this pull request Mar 20, 2017
PR-URL: #11594
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Mar 20, 2017

Landed in 5425e0d...74c1e02

@jasnell jasnell closed this Mar 20, 2017
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
PR-URL: nodejs#11594
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
PR-URL: nodejs#11594
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@MylesBorins
Copy link
Contributor

This will need to be manually backported to v7.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants