Changes between Version 9 and Version 27 of Ticket #6581


Ignore:
Timestamp:
04/16/2018 03:07:47 PM (20 months ago)
Author:
kzar
Comment:

... it's difficult to catch up with everything that has been said.

Agreed, I've attempted to update the issue description and subject to help there whilst keeping impartial. Hope you don't mind Andrea.

This should block #6425, not the other way around.

You're right of course, but Trac has an annoying bug... next time this happens don't swap the blocking + blocked-by fields in one step. Otherwise I've found Trac shows errors each time the issue is modified!

FWIW my preference is to update the coding style rules to be clearer when to use const. But I agree with Manish and Sebastian that I don't think const is particularly helpful in JavaScript and so I'd prefer not to use an ESLint rule which forces it to be used everywhere it can.

Legend:

Unmodified
Added
Removed
Modified
  • Ticket #6581

    • Property Cc mjethani juliandoucette ire added
    • Property Blocked By changed from 6425 to
    • Property Summary changed from [meta] enforce the usage of `const` whenever it's possible to [meta] Clarify the rules for using const vs let in JavaScript code
    • Property Blocking changed from to 6425
  • Ticket #6581 – Description

    v9 v27  
    11=== Background === 
    2 This topic has been [https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js#newcode24 previously discussed], but it keeps coming up, from time to time, so it would be nice if we could find a definitive answer to this debate so that our linter can help us to avoid wasting time. 
     2While it used to be variables were declared with `var` when writing modern JavaScript you are given the choice between `var`, `let` and `const`. It's uncontroversial to say that `let` is generally an improvement on `var`, but there is some disagreement about when `const` is best used instead of `let`. 
    33 
    4 ---- 
     4`const` is used to define a constant reference, where as `let` a mutable one. Note that even with a constant reference the value might be mutable! Our [https://adblockplus.org/coding-style#javascript-modern current coding style rules] say: 
    55 
    6 In our [https://adblockplus.org/en/coding-style#javascript-modern list of Modern JavaScript rules] we have two points that easily lead to ambiguity and/or debates: 
     6> Use const for constant expressions that could as well have been inlined (e.g. static parameters, imported modules, etc.). 
    77 
    8   * Always use block-scoping (let / const), except when sharing global variables between scripts cannot be avoided. 
    9   * Use const for constant expressions that could as well have been inlined (e.g. static parameters, imported modules, etc.). 
    10  
    11 There are many expressions that can be inlined and also part of a block scope, `for (const value of items) {}` is [https://tc39.github.io/ecma262/#prod-ForDeclaration just one of those]. 
    12  
    13 It's quite reasonable to expect that whenever a developer writes `const ref = value` such immutable reference is meant to be guarded against undesired reassignments, guards also naturally provided by the  programming language itself. 
    14  
    15 [https://eslint.org/docs/rules/prefer-const Quoting ESLint], which we use to enforce our own coding styles: 
    16  
    17   * `const` declaration tells readers, “''this variable is never reassigned''”, reducing cognitive load and improving maintainability. 
    18  
    19 Indeed there is a `prefer-const` options that would put an end forever to any related debate on this matter (already encountered 3 times, without being necessarily my code, during my last 3 months). 
    20  
    21 This ticket is open for discussion and my proposal is to embrace that rule for the following benefits: 
    22  
    23   * we do not transpile, engines can benefit from `const` information 
    24   * our code is better guarded against accidental reassignments 
    25   * there is a proper ESLint rule instead of a ''per case'' debate 
     8Which, [https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js#newcode24 while agreed at the time], is somewhat ambiguous and has led to disagreements on codereviews.  
    269 
    2710 
    2811=== What to change === 
     12It's still undecided what we should change, some people have suggested we simply improve the wording of our style rules. Some suggestions: 
    2913 
    30 Enable `prefer-const` and use ESLint `--fix` to improve our code base. 
     141. "Use const only for constants, not for variables". 
     152. "Use const (only) for real constants (on the module/top-level) which not being re-assigned isn't coincidental or matter to change in the future, but would inherently break things worth to protect against." 
     163. "Never use const!" 
    3117 
    32 Change the second point in our code style guide as: 
     18Other people prefer that we would enable the [https://eslint.org/docs/rules/prefer-const prefer-const] ESLint rule to enforce `const` to be used everywhere it can be - and update the style rules to match. 
    3319 
    34   * use `const` wherever it is valid syntax and no reassignment is meant 
     20Let's discuss here and see if we can reach some conclusion. Whatever we decide it's preferable that we do so once here, instead of multiple times in code reviews going forward.