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: use arrow function to take advantage of lexical this? #7295

Closed
originalfoo opened this issue Jun 13, 2016 · 3 comments
Closed

http: use arrow function to take advantage of lexical this? #7295

originalfoo opened this issue Jun 13, 2016 · 3 comments
Labels
http Issues or PRs related to the http subsystem.

Comments

@originalfoo
Copy link
Contributor

  • Version: 6.2.1
  • Platform:
  • Subsystem: lib/_http_outgoing.js

In OutgoingMessage.prototype.end there's a self var that's only used once for the finish() function - would it be better to use an arrow function instead?

Current code: https://github.com/nodejs/node/blob/master/lib/_http_outgoing.js#L549

  var self = this;
  function finish() {
    self.emit('finish');
  }

Proposed change:

let finish = () => this.emit('finish'); // lexical `this`

Also, as the finish() function isn't used until the code around L584, would it be worth moving the function closer to where it's used (along with the callback check) to aid code clarity?

// keep these 3 blocks together as they are related

let finish = () => this.emit('finish');

if (typeof callback === 'function')
  this.once('finish', callback);

if (this._hasBody && this.chunkedEncoding) {
  ret = this._send('0\r\n' + this._trailer + '\r\n', 'latin1', finish);
} else {
  // Force a flush, HACK.
  ret = this._send('', 'latin1', finish);
}
@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Jun 14, 2016
@Fishrock123
Copy link
Contributor

Fishrock123 commented Jun 16, 2016

We'd be happy to review a pull request if you'd like to make one. :)

@originalfoo
Copy link
Contributor Author

Will do 👍

originalfoo added a commit to originalfoo/node that referenced this issue Jun 23, 2016
Code relating to the `finish` event was split in to two areas of the
parent function. Gathered it together to clarify association within the
script.

Fixes nodejs#7295
@originalfoo
Copy link
Contributor Author

PR sent :)

Fishrock123 pushed a commit that referenced this issue Jul 5, 2016
Take advantage of arrow function lexical `this` to avoid defining a
`self = this` var which was only used once.

Code relating to the `finish` event was split in to two areas of the
parent function. Gathered it together to clarify association within the
script.

Fixes: #7295
PR-URL: #7378
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Brian White <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

No branches or pull requests

3 participants