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

cluster: remove bind() and self #7710

Merged
merged 1 commit into from
Jul 15, 2016
Merged

cluster: remove bind() and self #7710

merged 1 commit into from
Jul 15, 2016

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jul 13, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

cluster

Description of change

This commit removes the use of self and bind() from the cluster module in favor of arrow functions.

@nodejs-github-bot nodejs-github-bot added the cluster Issues and PRs related to the cluster subsystem. label Jul 13, 2016
@cjihrig
Copy link
Contributor Author

cjihrig commented Jul 14, 2016

R= @santigimeno ?

@targos
Copy link
Member

targos commented Jul 14, 2016

LGTM

@santigimeno
Copy link
Member

LGTM if CI is happy: https://ci.nodejs.org/job/node-test-pull-request/3295/

@JungMinu
Copy link
Member

LGTM

This commit removes the use of self and bind() from the cluster
module in favor of arrow functions.

PR-URL: nodejs#7710
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@cjihrig cjihrig merged commit 45367a2 into nodejs:master Jul 15, 2016
@cjihrig cjihrig deleted the self branch July 15, 2016 14:05
evanlucas pushed a commit that referenced this pull request Jul 19, 2016
This commit removes the use of self and bind() from the cluster
module in favor of arrow functions.

PR-URL: #7710
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
evanlucas pushed a commit that referenced this pull request Jul 20, 2016
This commit removes the use of self and bind() from the cluster
module in favor of arrow functions.

PR-URL: #7710
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@MylesBorins
Copy link
Contributor

@cjihrig I've backported this to v4.x. Do you know if there are any performance issues with arrow function in V8 4.8?

/cc @nodejs/v8

MylesBorins pushed a commit that referenced this pull request Sep 30, 2016
This commit removes the use of self and bind() from the cluster
module in favor of arrow functions.

PR-URL: #7710
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@cjihrig
Copy link
Contributor Author

cjihrig commented Sep 30, 2016

Nothing comes to mind. I don't recall the project ever holding back on the use of arrow functions for performance reasons.

@MylesBorins
Copy link
Contributor

@cjihrig sgtm. Just being extraaaa careful 😄

@ofrobots
Copy link
Contributor

What is the motivation to backport to v4.x?

@MylesBorins
Copy link
Contributor

@ofrobots trying to keep the delta in cluster as small as possible. This is only to staging and can easily be removed if you don't think it should land.

rvagg pushed a commit that referenced this pull request Oct 18, 2016
This commit removes the use of self and bind() from the cluster
module in favor of arrow functions.

PR-URL: #7710
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
This commit removes the use of self and bind() from the cluster
module in favor of arrow functions.

PR-URL: #7710
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Oct 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cluster Issues and PRs related to the cluster subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants