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

eslintrc,lib,test: enable prefer-const rule #3152

Closed
wants to merge 2 commits into from
Closed

eslintrc,lib,test: enable prefer-const rule #3152

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
af82367
tools: enable prefer-const eslint rule
thefourtheye Oct 8, 2015
06d5aee
lib,test: update let to const where applicable
thefourtheye Oct 8, 2015
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
5 changes: 5 additions & 0 deletions .eslintrc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,11 @@ rules:
# require space after keywords, eg 'for (..)'
space-after-keywords: 2

# ECMAScript 6
# list: http://eslint.org/docs/rules/#ecmascript-6
## Suggest using 'const' wherever possible
prefer-const: 2

# Strict Mode
# list: https://github.com/eslint/eslint/tree/master/docs/rules#strict-mode
## 'use strict' on top
Expand Down
2 changes: 1 addition & 1 deletion lib/cluster.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ function masterInit() {
var match = execArgv[i].match(/^(--debug|--debug-(brk|port))(=\d+)?$/);

if (match) {
let debugPort = process.debugPort + debugPortOffset;
const debugPort = process.debugPort + debugPortOffset;
++debugPortOffset;
execArgv[i] = match[1] + '=' + debugPort;
}
Expand Down
2 changes: 1 addition & 1 deletion lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,7 +517,7 @@ function formatCollectionIterator(ctx, value, recurseTimes, visibleKeys, keys) {
var nextRecurseTimes = recurseTimes === null ? null : recurseTimes - 1;
var vals = mirror.preview();
var output = [];
for (let o of vals) {
for (const o of vals) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does const really work for looping variables?

Copy link
Member

Choose a reason for hiding this comment

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

This introduces a bug.

var vals = [1,2,3];
for (const o of vals) console.log(o) // 1,1,1
for (let q of vals) console.log(q) // 1,2,3

EDIT: ...er, uh, only in the REPL, though?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct. Looks like it doesn't. I tried

for (const a of [1.3, 2, 3, 4]) {
  console.log(a);
}

and it prints only 1.3. Looks like the rule is buggy.

Copy link
Member

Choose a reason for hiding this comment

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

Apparently not:

~ % node
> var a = [0,1,2,3,4]
undefined
> for (const x of a){
... console.log(x)
... }
0
0
0
0
0
undefined
>

Copy link
Member

Choose a reason for hiding this comment

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

Hmm... it's a change in behavior (that is, a bug), but only in the REPL. If I put the code in a file, it works as expected.

Copy link
Member

Choose a reason for hiding this comment

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

Things like this do make me wonder if this is a wise rule to implement after all. The main argument for using const is that it lessens the cognitive load. You know that variable won't change. Except...uh...yeah....this. I think this actually makes the code harder to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, Let's park this till we find the actual reason why this is happening. What do you guys think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say it's definitely not safe (and likely against spec) to use const for loop vars and this looks like a rule and/or v8 bug to me. Spidermonkey also says no to this:

< for (const a of [1,2,3]) console.log(a)
> SyntaxError: invalid for/in left-hand side

Copy link
Member

Choose a reason for hiding this comment

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

According to this test each iteration is supposed to create a fresh binding, so it looks like a limitation in V8's sloppy mode. Behavior in strict mode is correct.

Copy link

Choose a reason for hiding this comment

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

const for a loop variable is completely valid - the binding is const inside the loop body and updated for each loop iteration. The non-strict behavior of const in V8 is a holdover from its original non-ES6 implementation of the feature.

Spidermonkey's error is from them not having implemented the feature.

output.push(formatValue(ctx, o, nextRecurseTimes));
}
return output;
Expand Down
13 changes: 7 additions & 6 deletions test/parallel/test-async-wrap-check-providers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,16 +73,17 @@ process.on('SIGINT', () => process.exit());

// Run from closed net server above.
function checkTLS() {
let options = {
const options = {
key: fs.readFileSync(common.fixturesDir + '/keys/ec-key.pem'),
cert: fs.readFileSync(common.fixturesDir + '/keys/ec-cert.pem')
};
let server = tls.createServer(options, noop).listen(common.PORT, function() {
tls.connect(common.PORT, { rejectUnauthorized: false }, function() {
this.destroy();
server.close();
const server = tls.createServer(options, noop)
.listen(common.PORT, function() {
tls.connect(common.PORT, { rejectUnauthorized: false }, function() {
this.destroy();
server.close();
});
});
});
}

zlib.createGzip();
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-buffer-zero-fill-reset.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,6 @@ function testUint8Array(ui) {

for (let i = 0; i < 100; i++) {
new Buffer(0);
let ui = new Uint8Array(65);
const ui = new Uint8Array(65);
assert.ok(testUint8Array(ui), 'Uint8Array is not zero-filled');
}
2 changes: 1 addition & 1 deletion test/parallel/test-http-flush-headers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ server.on('request', function(req, res) {
server.close();
});
server.listen(common.PORT, '127.0.0.1', function() {
let req = http.request({
const req = http.request({
method: 'GET',
host: '127.0.0.1',
port: common.PORT,
Expand Down
8 changes: 4 additions & 4 deletions test/parallel/test-http-response-multiheaders.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@ const norepeat = [
const server = http.createServer(function(req, res) {
var num = req.headers['x-num'];
if (num == 1) {
for (let name of norepeat) {
for (const name of norepeat) {
res.setHeader(name, ['A', 'B']);
}
res.setHeader('X-A', ['A', 'B']);
} else if (num == 2) {
let headers = {};
for (let name of norepeat) {
const headers = {};
for (const name of norepeat) {
headers[name] = ['A', 'B'];
}
headers['X-A'] = ['A', 'B'];
Expand All @@ -44,7 +44,7 @@ server.listen(common.PORT, common.mustCall(function() {
{port:common.PORT, headers:{'x-num': n}},
common.mustCall(function(res) {
if (n == 2) server.close();
for (let name of norepeat) {
for (const name of norepeat) {
assert.equal(res.headers[name], 'A');
}
assert.equal(res.headers['x-a'], 'A, B');
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-tls-socket-default-options.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ function testSocketOptions(socket, socketOptions) {
setImmediate(runTests);
});
}).listen(common.PORT, function() {
let c = new tls.TLSSocket(socket, socketOptions);
const c = new tls.TLSSocket(socket, socketOptions);
c.connect(common.PORT, function() {
c.end(sent);
});
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-util-inspect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ map.set(1, 2);
var mirror = Debug.MakeMirror(map.entries(), true);
var vals = mirror.preview();
var valsOutput = [];
for (let o of vals) {
for (const o of vals) {
valsOutput.push(o);
}

Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-zlib-truncated.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,11 @@ const inputString = 'ΩΩLorem ipsum dolor sit amet, consectetur adipiscing el'
].forEach(function(methods) {
zlib[methods.comp](inputString, function(err, compressed) {
assert(!err);
let truncated = compressed.slice(0, compressed.length / 2);
const truncated = compressed.slice(0, compressed.length / 2);

// sync sanity
assert.doesNotThrow(function() {
let decompressed = zlib[methods.decompSync](compressed);
const decompressed = zlib[methods.decompSync](compressed);
assert.equal(decompressed, inputString);
});

Expand Down
4 changes: 2 additions & 2 deletions test/sequential/test-child-process-fork-getconnections.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ const net = require('net');
const count = 12;

if (process.argv[2] === 'child') {
let sockets = [];
const sockets = [];

process.on('message', function(m, socket) {
function sendClosed(id) {
Expand Down Expand Up @@ -42,7 +42,7 @@ if (process.argv[2] === 'child') {
});

const server = net.createServer();
let sockets = [];
const sockets = [];
let sent = 0;

server.on('connection', function(socket) {
Expand Down