enabling eslint's prefer-const rule

[misterdjules]

This conversation started here: #3036 (comment).

It seems there's an implicit rule of preferring const over var for variables that hold values that never change, which makes sense. It's been mentioned in #1243, and seems to be enforced during code reviews.

However, it's not currently caught by our eslint setup. To paraphrase #3036 (comment), I'm not a big fan of having implicit coding style rules. It becomes frustrating for maintainers to enforce them every time, and for contributors because they don't have any tool to check that their code complies to the guidelines.

However, using the prefer-const rule does not apply to function-scoped variable declarations, so we would need to use the no-var eslint rule too and use let instead of var, which I assume would require a significant amount of work.

#1243 mentions that let's V8's implementation had performance at some point.

My question thus is: is moving from var to let and enforcing const with eslint worth investigating now?

/cc @nodejs/tsc

[mscdex]

FWIW I just ran a benchmark on jsperf.com and it looks like let performs the same as var with v8 4.5.

[trevnorris]

please done use those. they're useless on a benchmark of this scale.

Right now in the general case let performance is comparable to var. But there are specific scenarios where it completely fails and is much slower. One such case is when a class is defined within the same scope where let is used. Here's the generated output from v8's optimizing compiler, and you can see the let case generates 100+ more lines of instructions for the simple for loop: https://gist.github.com/trevnorris/73b0d8c0e831d6e48caa

[Fishrock123]

The argument against this is to slowly move to using const where possible, rather than making a massive diff that screws up git blame. (I am favor of moving slowly to it)

[trevnorris]

@Fishrock123 makes an appropriate point. This is how we've migrated code in the past. Unless the code was breaking for some reason, we've generally left these types of changes to organically migrate through the code base.

Though how do we then address new PRs? I think it's a deterrent for new contributors when they are barrage with nit style comments. In the past I've made changes to the commits before landing it. Though that has always been for minor things (e.g. trailing whitespace). Not sure how we'd feel about doing the same for the many variable declarations.

[Trott]

If no-var is off the table (and it sounds like it is, at least for the foreseeable future), turning on prefer-const by itself only flags five lines in four files of the current code base. I'm actually all for turning it on.

[Fishrock123]

@Trott could you get us a diff? :)

[thefourtheye]

➜  io.js git:(master) ✗ make lint
./node tools/eslint/bin/eslint.js src lib test --rulesdir tools/eslint-rules --reset --quiet

lib/cluster.js
  294:12  error  `debugPort` is never modified, use `const` instead  prefer-const

test/parallel/test-buffer-zero-fill-reset.js
  17:6  error  `ui` is never modified, use `const` instead  prefer-const

test/parallel/test-http-flush-headers.js
  13:6  error  `req` is never modified, use `const` instead  prefer-const

test/sequential/test-child-process-fork-getconnections.js
   9:6  error  `sockets` is never modified, use `const` instead  prefer-const
  45:6  error  `sockets` is never modified, use `const` instead  prefer-const

✖ 5 problems (5 errors, 0 warnings)

make: *** [jslint] Error 1
➜  io.js git:(master) ✗ git --no-pager diff
diff --git a/.eslintrc b/.eslintrc
index cf1f768..eeaaff4 100644
--- a/.eslintrc
+++ b/.eslintrc
@@ -70,6 +70,8 @@ rules:
   # require space after keywords, eg 'for (..)'
   space-after-keywords: 2

+  prefer-const: 2
+
   # Strict Mode
   # list: https://github.com/eslint/eslint/tree/master/docs/rules#strict-mode
   ## 'use strict' on top

[cjihrig]

Since this is almost entirely in tests, I'm +1 for the change.

[Fishrock123]

I have a hard time believing those are the only spots, could it be the rule isn't that good at picking things up?

[Trott]

@Fishrock123 The rule only applies to 'let' declarations and not 'var'. 'const' would change the scope of a 'var' but not a 'let'.) So that's why the number of changes would be small.

[rvagg]

The argument against this is to slowly move to using const where possible, rather than making a massive diff that screws up git blame. (I am favor of moving slowly to it)

This is frustrating because it leaves us in the land of the implicit which is terrible unless everyone's on the same page, which we are not. I don't recall a discussion where it was agreed that we were moving to const slowly and now we are in a situation where a subgroup of the collaborators push on this when they are reviewing PRs and others don't leading to uncertainty, like in #2411, which is an unpleasant experience for contributors (and collaborators!).

Either it's explicit and stated somewhere (do we need a styleguide we can bikeshed over?) or it should be left up to contributors and not haggled over by reviewers.

[ChALkeR]

If no-var is off the table (and it sounds like it is, at least for the foreseeable future), turning on prefer-const by itself only flags five lines in four files of the current code base. I'm actually all for turning it on.

I'm +1 for turning prefer-const on in such situation.

Above all, it gives a clear hint to any person who is looking on the code that the variable isn't going to be re-assigned. It has at least that advantage over var.

[thefourtheye]

@rvagg I am sorry. That was me who suggested it in #2411. I just thought its more appropriate to use const there. I'll hold off on suggesting it till we reach a conclusion.