GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — child_process: refactor self=this in socket_list by benjamingr · Pull Request #5860 · nodejs/node

Conversation

@benjamingr

@benjamingr benjamingr commented Mar 23, 2016

Copy link
Copy Markdown
Member

Pull Request check-list

Affected core subsystem(s)

child_process

Description of change

The socket list module (used by child_process) currently uses the var self = this; pattern for context in several places, this PR replaces this with arrow functions or passing a parameter in where appropriate.

Note that the var self = this in the _request is intentionally left in place since it is not trivial to refactor it and the current pattern isn't bad given the use case.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@benjamingr
child_process: refactor self=this in socket_list
4ff1009 
The socket list module (used by child_process) currently uses the
`var self = this;` pattern for context in several places, this PR
replaces this with arrow functions or passing a parameter in where
appropriate.

Note that the `var self = this` in the _request is intentioanlly
left in place since it is not trivial to refactor it and the current
pattern isn't bad given the use case.

PR-URL:
Reviewed-By:
Reviewed-By:
@benjamingr benjamingr added the child_process Issues and PRs related to the child_process subsystem. label Mar 23, 2016
@mscdex

mscdex commented Mar 23, 2016

Copy link
Copy Markdown
Contributor

LGTM if CI is ok: https://ci.nodejs.org/job/node-test-pull-request/2039/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cjihrig

cjihrig commented Mar 23, 2016

Copy link
Copy Markdown
Contributor

LGTM, and CI is green.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Mar 23, 2016

Copy link
Copy Markdown
Member

LGTM, CI is green
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell added the lts-watch-v4.x label Mar 23, 2016
Fishrock123
Fishrock123 reviewed Mar 23, 2016
View reviewed changes
Comment thread lib/internal/socket_list.js
this.connections--;

if (self.connections === 0) self.emit('empty');
if (this.connections === 0) this.emit('empty', this);

@Fishrock123 Fishrock123 Mar 23, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Where does the passed this end up here? Seems unnecessary?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@mscdex mscdex Mar 23, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It gets passed to onempty() which is the empty event handler added on line 82.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
benjamingr and Fishrock123 reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

@ludoblues ludoblues Mar 24, 2016

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

To send the context as a parameter seems ambigous. Since we are using arrow functions, why not just binding the method l.82 ? onempty.bind(this)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@benjamingr benjamingr Mar 27, 2016

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@ludoblues bind is slow and it makes the resulting function slow too. It is also very scarcely used in Node where this pattern is very common. See http://stackoverflow.com/questions/17638305/why-is-bind-slower-than-a-closure

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@benjamingr

benjamingr commented Mar 27, 2016

Copy link
Copy Markdown
Member Author

Landed in a6b9b55
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@benjamingr benjamingr closed this Mar 27, 2016
benjamingr added a commit that referenced this pull request Mar 27, 2016
@benjamingr
child_process: refactor self=this in socket_list
a6b9b55 
The socket list module (used by child_process) currently uses the
`var self = this;` pattern for context in several places, this PR
replaces this with arrow functions or passing a parameter in where
appropriate.

Note that the `var self = this` in the _request is intentioanlly
left in place since it is not trivial to refactor it and the current
pattern isn't bad given the use case.

PR-URL: #5860
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Brian White <[email protected]>
evanlucas pushed a commit that referenced this pull request Mar 30, 2016
@benjamingr @evanlucas
child_process: refactor self=this in socket_list
459a7d6 
The socket list module (used by child_process) currently uses the
`var self = this;` pattern for context in several places, this PR
replaces this with arrow functions or passing a parameter in where
appropriate.

Note that the `var self = this` in the _request is intentioanlly
left in place since it is not trivial to refactor it and the current
pattern isn't bad given the use case.

PR-URL: #5860
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Brian White <[email protected]>
@evanlucas evanlucas mentioned this pull request Mar 31, 2016
v5.10.0 Release Proposal #5970
Closed
evanlucas pushed a commit that referenced this pull request Mar 31, 2016
@benjamingr @evanlucas
child_process: refactor self=this in socket_list
660ec9f 
The socket list module (used by child_process) currently uses the
`var self = this;` pattern for context in several places, this PR
replaces this with arrow functions or passing a parameter in where
appropriate.

Note that the `var self = this` in the _request is intentioanlly
left in place since it is not trivial to refactor it and the current
pattern isn't bad given the use case.

PR-URL: #5860
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Brian White <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 8, 2016
@benjamingr Myles Borins
child_process: refactor self=this in socket_list
d275cdf 
The socket list module (used by child_process) currently uses the
`var self = this;` pattern for context in several places, this PR
replaces this with arrow functions or passing a parameter in where
appropriate.

Note that the `var self = this` in the _request is intentioanlly
left in place since it is not trivial to refactor it and the current
pattern isn't bad given the use case.

PR-URL: #5860
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Brian White <[email protected]>
@MylesBorins MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Apr 8, 2016
@MylesBorins MylesBorins mentioned this pull request Apr 11, 2016
V4.4.3 proposal #6149
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

child_process Issues and PRs related to the child_process subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@benjamingr @mscdex @cjihrig @jasnell @Fishrock123 @ludoblues @MylesBorins
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.