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.

Sign up for GitHub

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

Jump to bottom

various fix in lib #26562

Closed
wants to merge 3 commits into from
Closed

various fix in lib #26562

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2cc0cfb
http: delay ret declaration in method _flushOutput
gengjiawen Mar 10, 2019
4bd2635
repl: remove redundant initialization
gengjiawen Mar 10, 2019
d768c5c
lib: make lowerProto scope more clear
gengjiawen Mar 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions lib/_http_outgoing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -792,13 +792,13 @@ OutgoingMessage.prototype._flush = function _flush() {
};

OutgoingMessage.prototype._flushOutput = function _flushOutput(socket) {
var ret;
var outputLength = this.outputData.length;
const outputLength = this.outputData.length;
if (outputLength <= 0)
return ret;
return undefined;

var outputData = this.outputData;
const outputData = this.outputData;
socket.cork();
let ret;
for (var i = 0; i < outputLength; i++) {
Copy link
Member Author

Choose a reason for hiding this comment

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

I change var to let in my first try. But I got

/home/travis/build/nodejs/node/lib/_http_outgoing.js
  802:8  error  Use of `let` as the loop variable in a for-loop is not recommended. Please use `var` instead  node-core/no-let-in-for-declaration

Can we remove this rule in master branch ?
cc @BridgeAR @Trott

Copy link
Member

Choose a reason for hiding this comment

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

It has been two years since #8637 (when using let/const lost performance), and let/const performance in V8 was improved due to #8637 (comment) and #9729 (comment)

I think maybe it's time to remove this rule now ?

Copy link
Member

@Trott Trott Mar 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

That rule was instituted because using let with a for loop that way could be drastically slower than using var. The problem was fixed in V8 6.0.

This performance problem still exists in Node.js 6.x (which ships with V8 5.1). Since code that lands on the master branch can be backported to 6.x, the rule remains in place. Support for 6.x ends this June, so starting in July, we can disable the rule. At that time, the oldest supported Node.js version will be 8.x, which currently ships with V8 6.2.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or start a pr and add the label don't land on 6.x ?

Copy link
Member

Choose a reason for hiding this comment

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

6.x is in maintenance. I think we can already disable the rule.

const { data, encoding, callback } = outputData[i];
ret = socket.write(data, encoding, callback);
Expand Down
3 changes: 1 addition & 2 deletions lib/repl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1030,8 +1030,7 @@ function complete(line, callback) {

// REPL commands (e.g. ".break").
var filter;
var match = null;
match = line.match(/^\s*\.(\w*)$/);
let match = line.match(/^\s*\.(\w*)$/);
if (match) {
completionGroups.push(Object.keys(this.commands));
completeOn = match[1];
Expand Down
5 changes: 3 additions & 2 deletions lib/url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,10 +250,11 @@ Url.prototype.parse = function parse(url, parseQueryString, slashesDenoteHost) {
}
}

var proto = protocolPattern.exec(rest);
let proto = protocolPattern.exec(rest);
let lowerProto;
if (proto) {
proto = proto[0];
var lowerProto = proto.toLowerCase();
lowerProto = proto.toLowerCase();
this.protocol = lowerProto;
gengjiawen marked this conversation as resolved.
Show resolved Hide resolved
rest = rest.slice(proto.length);
}
Expand Down