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

net: refactor self=this to arrow functions #5857

Closed
wants to merge 2 commits into from

Conversation

benjamingr
Copy link
Member

Affected core subsystem(s)

net

Description of change

Refactor unused self=this code to code without without this pattern
making it more consistent with the rest of our code.

There were PRs to do this code-base wide but they all seem to have stalled so I figured I'd do this in the least objectionable way one change at a time.

CI: https://ci.nodejs.org/job/node-test-pull-request/2037/console

Refactor unused self=this code to code without without this pattern
making it more consistent with the rest of our code.

PR-URL:
Reviewed-By:
Reviewed-By:
@benjamingr benjamingr added net Issues and PRs related to the net subsystem. lts-watch-v4.x labels Mar 23, 2016
}
}

if (this.destroyed) {
debug('already destroyed, fire error callbacks');
fireErrorCallbacks();
fireErrorCallbacks.call(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

What about just passing this to the function and have self be the parameter name?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's actually probably a better idea. Thanks.

@mscdex
Copy link
Contributor

mscdex commented Mar 23, 2016

One nit but otherwise LGTM.

@@ -485,7 +483,7 @@ Socket.prototype._destroy = function(exception, cb) {
// to make it re-entrance safe in case Socket.prototype.destroy()
// is called within callbacks
this.destroyed = true;
fireErrorCallbacks();
fireErrorCallbacks.call(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

@benjamingr
Copy link
Member Author

Addressed nit, thanks.

@cjihrig
Copy link
Contributor

cjihrig commented Mar 23, 2016

LGTM

3 similar comments
@Fishrock123
Copy link
Contributor

LGTM

@r-52
Copy link
Contributor

r-52 commented Mar 23, 2016

LGTM

@jasnell
Copy link
Member

jasnell commented Mar 23, 2016

LGTM

benjamingr added a commit that referenced this pull request Mar 27, 2016
Refactor unused self=this code to code without without this pattern
making it more consistent with the rest of our code.

PR-URL: #5857
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Roman Klauke <romankl@users.noreply.github.com>
@benjamingr
Copy link
Member Author

Landed in a15906c , thanks reviewers :)

@benjamingr benjamingr closed this Mar 27, 2016
@evanlucas
Copy link
Contributor

This is not applying cleanly to v5.x. Could you backport it please?

MylesBorins pushed a commit that referenced this pull request Apr 5, 2016
Refactor unused self=this code to code without without this pattern
making it more consistent with the rest of our code.

PR-URL: #5857
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Roman Klauke <romankl@users.noreply.github.com>
This was referenced Apr 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants