lib: var to const

[BridgeAR]

As discussed in #13732: this will change all var to const if applicable.
The main question is if this is wanted due to the churn.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

lib

[tniessen]

I don't have a strong opinion on this if someone can confirm that there is no performance degression.

[tniessen]

CI: https://ci.nodejs.org/job/node-test-pull-request/8723/

[BridgeAR]

@tniessen I just compared the streams and the performance did not change:

 streams/readable-bigread.js n=1000                        -1.92 %            0.4905409
 streams/readable-bigunevenread.js n=1000                   1.85 %            0.3596399
 streams/readable-boundaryread.js type="buffer" n=2000      1.20 %            0.6028271
 streams/readable-boundaryread.js type="string" n=2000      2.73 %            0.2102648
 streams/readable-readall.js n=5000                         1.21 %            0.4121098
 streams/readable-unevenread.js n=1000                      1.34 %            0.5934662
 streams/transform-creation.js n=1000000                   -1.38 %            0.6556022
 streams/writable-manywrites.js n=2000000                   2.05 %            0.7847301

[refack]

Ref: #13729

[gibfahn]

So it's probably worth thinking about backporting here. If this cannot be backported to v6.x due to deoptimisations then that will make it very difficult to backport any other changes after this.

Maybe the answer is to wait until v6.x goes into maintenance. I'm +1 on removing var where possible, but backporting is already a massive amount of work, and I think this would only make it harder.

Thoughts @nodejs/lts ?

[BridgeAR]

@gibfahn I am running the deopt test in v6 with and without all vars -> const. This might give us a better insight in what impact this would have.

[BridgeAR]

Just as info - I am going to update this as soon as v8 6.1 lands. The deopt test does not work anymore with Turbofan as there are pretty much no deopts but bmeurer (not mentioning him here because he does not need to be summoned for it) fixed a deopt in #13403 (comment) and as he said that will be fixed with 6.1. So I think we are pretty safe going forward from that point on.

[BridgeAR]

Rebased. As v8 6.1 has landed this can now also properly land.

[gibfahn]

Another ping @nodejs/lts re backporting concerns for this. I'm still thinking it might be better to wait for April 2018 when Node 6.x goes maintenance, but that's also a long time to wait.

It's annoying that there isn't an eslint rule we can add for this, no-var will force var->let, and prefer-const only checks let->const.

I guess we could enable prefer-const in the top-level eslint file (it won't catch var -> const, but it's the first half).

[BridgeAR]

@gibfahn prefer-const is already in the top-level eslint file and I actually requested a rule improvement for this use case but it did not get enough attention 😞.

I do see your point about the churn in general even though we did some other major steps like this before as well.

[jasnell]

This LGTM but I'd like to see some benchmarks and a CITGM run.

[BridgeAR]

@jasnell any const that might be reassigned would trigger (or at least should) an eslint error. Those var statements were in fact changed by eslint and not by me (that would be a horrible amount of work...) so two rules would have to make a mistake here. a) the prefer-const rule and b) no-const-assign rule.

I am also not sure how CITGM could show any issues because all those variables can not be reassigned by a user. Another main aspect is that the tests would very likely fail if there is a reassignment for any const because we have a quite decent coverage for such changes.

I would like to get this landed soon because the code base changes fast and I would have to rebase often otherwise.

[jasnell]

Indeed, no arguments there. CITGM is not a blocker, it's just advisory just in case.
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/989/

[BridgeAR]

I just thought about @gibfahn s comment again about backporting... It is indeed going to make our lives miserable to backport things to v6.x. We should not backport this PR to v6.x because there are some deoptimizations possible in some cases and therefore it would increase the amount of conflicts for many backports. So...
Even though I just pushed for landing this soon I guess our lives are going to be easier if something like this will not land before May 2018. We could already port all tests and only accept let and const in those as we can safely backport that right away (we do not have to care about the performance in our tests on v6.x). The benchmarks are also fine (const only for now - I still have to check if let is finally on par with var performance wise in all cases) since those are not changed as often.

I believe you would be much happier with that? @gibfahn

[gibfahn]

I believe you would be much happier with that? @gibfahn

Happier is the wrong word, I'd personally rather we could use const 😁 . But yeah, leaving lib for another 6 months is probably the right call.

I'll put this on the LTS (now Release) agenda, and we'll discuss at the next meeting.

[sam-github]

Agreed to wait 6 months to do this sweep. At that point, 6.x will be in maintenance, and this can be applied to master and backported to 8.x safely at that time.

[BridgeAR]

Thx closing this in that case.