enabling eslint's prefer-const rule #3118
Comments
|
FWIW I just ran a benchmark on jsperf.com and it looks like |
|
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 |
|
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) |
|
@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. |
|
If |
|
@Trott could you get us a diff? :) |
|
|
Since this is almost entirely in tests, I'm +1 for the change. |
|
I have a hard time believing those are the only spots, could it be the rule isn't that good at picking things up? |
|
@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. |
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. |
I'm +1 for turning 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 |
|
I'm with @rvagg on this. The current approach is haphazard at best. We need
|
|
We could switch everything to
Also, let's not forget #2286. |
That has the potential to break things due to block scoping — I might suggest doing a sweep with
If possible, I'd prefer we go the |
|
@chrisdickinson I think these scope issues can be caught beforehand through the |
Well, not necessarily, not everything can be caught by linters. :) |
|
Forcing let is a definitive -1. I've already demonstrated there are corner cases where let fails on performance. To take a potential performance hit so we can feel warm and fuzzy about our code style is unacceptable. |
|
It seems like that's worth a cleanup issue!
True! Style concerns can be caught, though — eslint is pretty expressive when it comes to making new rules, and building these opinions into our linter is just about the only way to make sure they're consistently applied in the long run. (Plus, leaning on the linter saves reviewer time, and reduces back-and-forth in the contribution process!)
Given that the codebase will survive multiple versions of V8, how long do we expect to see the |
We don't code for what will be optimized, but what is optimized. And I have no idea when it will be fixed.
In for loops it is substantial, and I don't have a conclusive list of every case where this is applicable. As you can see from my test case, the optimizing compiler completely fails. Generating almost twice the number of instructions. This is enough to turn from an annoyance to a blocking issue. Even if we don't hit a case today, and we don't know all cases where it may appear, what will happen if a change comes in that hits it after this lands? I'm +1 on making everything const that can be, but not at the cost of forcing let down everyone's throat. |
As per block-scoped-var rule, the variables which were used before declaration or declared within a block and used outside are trated as linter errors. Refer: nodejs#3118
As per block-scoped-var rule, the variables which were used before declaration or declared within a block and used outside are trated as linter errors. Refer: nodejs#3118
|
Question: why can't we prefer const to var where applicable instead of |
Description from: http://eslint.org/docs/rules/prefer-const.html If a variable is never modified, using the `const` declaration is better. `const` declaration tells readers, "this variable is never modified," reducing cognitive load and improving maintainability. Refer: nodejs#3118 PR-URL: nodejs#3152 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Roman Reiss <[email protected]>
As per the `prefer-const` eslint rule, few instances of `let` have been identified to be better with `const`. This patch updates all those instances. Refer: nodejs#3118 PR-URL: nodejs#3152 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Roman Reiss <[email protected]>
Description from: http://eslint.org/docs/rules/prefer-const.html If a variable is never modified, using the `const` declaration is better. `const` declaration tells readers, "this variable is never modified," reducing cognitive load and improving maintainability. Refer: #3118 PR-URL: #3152 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
As per the `prefer-const` eslint rule, few instances of `let` have been identified to be better with `const`. This patch updates all those instances. Refer: #3118 PR-URL: #3152 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
Description from: http://eslint.org/docs/rules/prefer-const.html If a variable is never modified, using the `const` declaration is better. `const` declaration tells readers, "this variable is never modified," reducing cognitive load and improving maintainability. Refer: nodejs#3118 PR-URL: nodejs#3152 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
As per the `prefer-const` eslint rule, few instances of `let` have been identified to be better with `const`. This patch updates all those instances. Refer: nodejs#3118 PR-URL: nodejs#3152 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
|
Closing this as |
As per the `prefer-const` eslint rule, few instances of `let` have been identified to be better with `const`. This patch updates all those instances. Refer: #3118 PR-URL: #3152 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
Description from: http://eslint.org/docs/rules/prefer-const.html If a variable is never modified, using the `const` declaration is better. `const` declaration tells readers, "this variable is never modified," reducing cognitive load and improving maintainability. Refer: #3118 PR-URL: #3152 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
As per the `prefer-const` eslint rule, few instances of `let` have been identified to be better with `const`. This patch updates all those instances. Refer: #3118 PR-URL: #3152 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
Description from: http://eslint.org/docs/rules/prefer-const.html If a variable is never modified, using the `const` declaration is better. `const` declaration tells readers, "this variable is never modified," reducing cognitive load and improving maintainability. Refer: #3118 PR-URL: #3152 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
I know this is closed, and there may be a more relevant discussion (someone please link?) but my solution is: |
|
we should ping folks on the V8 team to get an idea of why this is still an issue. it’s been several years now and this syntax isn’t exactly new. it might just be a matter of adding this case into some of the perf suites JS VM’s are running. |
It has been resolved for some time. Loops using |
what about |
Doh! I did the "see what I expected to see and not what is actually there on the screen in front of me" thing. Sorry. I don't know the answer to that. |
I don't know what sort of credibility jsperf has with the team here, but I created a test earlier today that addresses this question: https://jsperf.com/foreach-vs-for-of-performance-test/9 After my initial run, |
|
making sure @rvagg sees this so that he can stop frowning at my aggressive |
|
bleh, I'm just a slave to whatever |
This conversation started here: #3036 (comment).
It seems there's an implicit rule of preferring
constovervarfor 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
letinstead ofvar, 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
vartoletand enforcingconstwith eslint worth investigating now?/cc @nodejs/tsc
The text was updated successfully, but these errors were encountered: