Opened on 04/12/2018 at 04:54:13 PM

Closed on 05/19/2018 at 08:41:59 AM

Last modified on 05/22/2018 at 01:54:31 PM

#6581 closed change (worksforme)

[meta] Clarify the rules for using const vs let in JavaScript code

Reported by: agiammarchi Assignee:
Priority: Unknown Milestone:
Module: Unknown Keywords:
Cc: greiner, kzar, sebastian, saroyanm, sergz, mjethani, juliandoucette, ire Blocked By:
Blocking: #6425 Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description (last modified by kzar)

Background

While 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.

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 current coding style rules say:

Use const for constant expressions that could as well have been inlined (e.g. static parameters, imported modules, etc.).

Which, while agreed at the time, is somewhat ambiguous and has led to disagreements on codereviews.

What to change

It's still undecided what we should change, some people have suggested we simply improve the wording of our style rules. Some suggestions:

  1. "Use const only for constants, not for variables".
  2. "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."
  3. "Never use const!"

Other people prefer that we would enable the prefer-const ESLint rule to enforce const to be used everywhere it can be - and update the style rules to match.

Let'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.

Attachments (0)

Change History (51)

comment:1 Changed on 04/12/2018 at 04:57:20 PM by greiner

  • Cc greiner added

comment:2 Changed on 04/12/2018 at 05:06:34 PM by agiammarchi

  • Cc kzar sebastian saroyanm added
  • Component changed from Unknown to Platform

comment:3 Changed on 04/12/2018 at 05:08:44 PM by agiammarchi

  • Cc sergz added

comment:4 Changed on 04/12/2018 at 05:10:35 PM by agiammarchi

  • Description modified (diff)

comment:5 follow-ups: Changed on 04/13/2018 at 09:08:00 AM by kzar

For reference here's an old conversation we had of let vs const which I dug out yesterday: https://codereview.adblockplus.org/29370999/diff/29371943/messageResponder.js#newcode24

I think the issue description would be more useful if it started explaining a little background of let and const, then went to explain the problem of deciding to use one vs the other and our current rules and the thinking behind them. Then it could go on to raise the question of how we want to change things (if at all) to address those concerns.

Ultimately if we change our coding style rules and therefore ESLint configuration it's a group decision. It's good you've made a suggestion of how we could change things, since that's a useful place to start a discussion, but try not to let your own opinion dominate. "This ticket is open for discussion but..." - no this ticket _is_ a discussion.

comment:6 in reply to: ↑ 5 Changed on 04/13/2018 at 09:18:32 AM by agiammarchi

Replying to kzar:

"This ticket is open for discussion but..." - no this ticket _is_ a discussion.

absolutely. would an and be better there? there was no intention to shadow the ticket purpose, which is to discuss the rule and my proposal indeed.

comment:7 Changed on 04/13/2018 at 09:19:22 AM by agiammarchi

  • Description modified (diff)

comment:8 in reply to: ↑ 5 Changed on 04/13/2018 at 10:09:38 AM by agiammarchi

Replying to kzar:

I think the issue description would be more useful if it started explaining a little background of let and const,

I am not sure what kind of background you mention there, if it's the link you posted, I can put it at the beginning.

edit: done

then went to explain the problem of deciding to use one vs the other and our current rules and the thinking behind them.

the problem I've exposed is: the rules are ambiguous as described right now, and we're still indeed discussing in recent code reviews those rules without any automatic help from the linter.

Such linter community agreed on the importance of the usage of const though, and it provides a rule to enforce that but AFAIK there's nothing to enforce our current, per-case oriented, choice.

Then it could go on to raise the question of how we want to change things (if at all) to address those concerns.

If that's the only counter argument we have, we should also consider that review is based on CommonJS, which has no rules on how you'd import from it. However, we are planning to update our core code and style to use ES6 imports, which are all constants. Classes also are like constants. Accordingly, since we will have mutable data referenced as constant, the point about mutable values becomes IMO weak in these days, while it would be a pity to loose a language feature as const is.

In JavaScript const works simply like that, which is similar in Python, but not like raw C. But should we ditch every JS language feature that has different meaning in other PLs?

Because if we compare const with other programming languages, and we decide since it's different we shouldn't use it unless it's exactly identical in meaning, we should as well reconsider to not use class too, because also classes in JS are mutable by default (and freezing prototypes have side effects so it's impractical).

On the other hand, if to avoid debates I'd end up using always let to make my life decision simpler, developers reviewing my code will need to be more careful, following up all variables and, eventually, ask if a reassignment was meant or not. They also will surely nitpick some case for const instead of let, like it keeps happening these days, where a for (const x of inlineExpression) {}, as example, is even technically compatible with our current code style rules, but it was asked to be changed.

If we really want to keep avoiding const then, we need to improve the rule because it's currently ambiguous, at least for my understanding and others devs too.

Last, but not least, embracing prefer-const in ESLint, will better align our style with the community around ESLint, and will stop us nitpicking around this topic, for something also considered a best practice.

React, Angular, Vue, even jQuery community these days, also prefer const over let, why wouldn't we follow a reasonable trend that also makes our code more robust and our style more friendly for new comers and/or external contributors?

Last edited on 04/13/2018 at 10:14:23 AM by agiammarchi

comment:9 Changed on 04/13/2018 at 10:12:21 AM by agiammarchi

  • Description modified (diff)

comment:10 Changed on 04/14/2018 at 01:57:54 AM by sebastian

  • Cc mjethani added

I personally don't care about const in JavaScript. It's advantages (if any) are negligible. JavaScript engines do NOT benefit from it, since the JIT compiler already knows if a variable will be changed or not regardless how it was declared. It's mostly a hint for developers, indicating that a variable isn't re-assigned. How useful this information is is highly subjective. While some developers believe that they can better understand code if its obvious from the declaration of a variable whether it will ever be re-assigned, any language feature makes the language more complex. It's one more thing you have to think about, whether to use const or let, when writing code. Also when changing existing code and a variable that was defined with const before, now needs to be re-assigned, you might find yourself changing variable declarations from const to let (and potentially later back to const), all the time.

Anyway, what is more important here, is having a rule that is unambiguous, easy to enforce consistently, and avoids subjective debates in code reviews. This leaves us with any of the following options:

  • Never use const.
  • Only use const in few well-defined uncontroversial scenarios (e.g. for module imports, aliases, numerical identifiers, hard-coded values). This was the intention of our current rule.
  • Use const wherever possible (enforced by ESlint's prefer-const rule), as suggested here.

I'm happy to go with whichever everybody else agrees on.

Last edited on 04/14/2018 at 02:04:19 AM by sebastian

comment:11 follow-up: Changed on 04/14/2018 at 02:29:27 PM by mjethani

As I had said in an earlier discussion on IRC, I don't find const to be very useful in the current version of JavaScript. It doesn't seem to have any performance benefits. To me it's more like an assertion to make your assumptions about the code explicit. If your logic depends on the value of a variable remaining constant then it's OK to use const to make that assumption explicit. This is so that the next person reading the code knows about this assumption. In that case, changing the const to a let should be a big deal. You must be able to explain why you are dropping the assumption. Did the algorithm change? Why did it change? What are the effects of this change on the overall program? Should new tests be written?

If any variable that simply happens not to be reassigned is declared as a const then the keyword loses its purpose. How can you tell the difference between variables that are intended to be const and those that are const only incidentally? It makes no sense to use const everywhere. const should be used only when it is an assumption in the logic, i.e. the algorithm dictates that the value must never change.

Our policy should be to use let everywhere and use const only for the cases where there's a strong assumption that the value must remain constant. If you cannot explain why you've declared something as const, you should leave it as let.

A good example of const:

const elemhideRegExp = /^([^/*|@"!]*?)#([@?])?#(.+)$/;

This is the regular expression that identifies a filter as an element hiding filter. If it changes during the course of the program's execution, it'll break things. It should be const.

(You'll notice that in reality this is a property on the Filter object. Ideally you would be able to do something like const Filter.elemhideRegExp = /^([^/*|@"!]*?)#([@?])?#(.+)$/;, which would create a non-writable property on the object, but that's why I said that the const keyword in JavaScript is a half-baked feature of a language that is dynamic by design. You can't ensure constness in most cases that matter without writing too much code, so why bother at all?)

A bad example of const:

function updateElements()
{
  const elementCount = getNumberOfElements();
  for (let i = 0; i < elementCount; i++)
  {
    let element = getNextElement();
    doSomethingWithElement(element);
  }
  return elementCount;
}

Now suppose we had to change the code to this:

function updateElements()
{
  let elementCount = getNumberOfElements();
  for (let i = 0; i < elementCount; i++)
  {
    let element = getNextElement();
    if (!doSomethingWithElement(element))
    {
      deleteElement(element);
      elementCount--;
    }
  }
  return elementCount;
}

So we changed const to let because we had to decrement the value. But why was it const in the first place!? There was no good reason to make it const! Nothing would break if the value changed. In such cases const should not be used.

I personally don't find it difficult to decide when to use const. If it's too much cognitive overhead, I would agree that it is not for much benefit. When have we "accidentally" modified a value that could have been avoided with the use of const? As I showed with the example of Filter.elemhideRegExp, in most cases where it matters we cannot use const anyway. We might as well not use it at all or use it in the very rare cases where it would really help avoid trouble.

comment:12 Changed on 04/14/2018 at 02:35:43 PM by mjethani

  • Blocked By 6425 removed
  • Blocking 6425 added
  • Summary changed from [meta] enforce the usage of `const` whenever it's possible to [meta] Enforce the usage of const whenever it's possible

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

comment:13 Changed on 04/14/2018 at 04:59:18 PM by mjethani

I think this blog post says it very well: https://madhatted.com/2016/1/25/let-it-be

We should update that line in the style guide: "Use const only for constants, not for variables". It's quite simple. In for (let item of array) {}, item is still a variable, just because it is not reassigned doesn't make it a constant. In let PI = 3.14;, PI is a constant, assigning any other value to it would break things; in this case, the let keyword may be replaced with const to make it clear.

Last edited on 04/14/2018 at 05:19:40 PM by mjethani

comment:14 Changed on 04/14/2018 at 09:18:38 PM by sebastian

I tend to agree with mjethani (and the article he refers to). Still I wonder how to turn that into an unambiguous rule for our coding style guide. Maybe something like this:

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.

Last edited on 04/14/2018 at 10:05:49 PM by sebastian

comment:15 in reply to: ↑ 11 ; follow-up: Changed on 04/16/2018 at 08:39:12 AM by agiammarchi

Replying to mjethani:

How can you tell the difference between variables that are intended to be const and those that are const only incidentally?

you can ask the same question about let

It makes no sense to use const everywhere.

you can say the same about let

const should be used only when it is an assumption in the logic, i.e. the algorithm dictates that the value must never change.

a for/of might explicitly have a logic that prefers the const guard.

Imagine a list of scalar values, if the argument is about what is a constant, then the for/of becomes natural choice.

Our policy should be to use let everywhere and use const only for the cases where there's a strong assumption that the value must remain constant.

This is free interpretation, hence the issue. I don't want to waste anyone time when I chose const for both me, deciding that is the right thing I want, and a developer that might need explanations.

I am sure we all have better things to do, and I want to be sure my linter fixes everything I do on CTRL+S (like I do already).

If you cannot explain why you've declared something as const, you should leave it as let.

I don't want spend my time explaining when I want a guard, why would other devs?

A good example of const:

const elemhideRegExp = /^([^/*|@"!]*?)#([@?])?#(.+)$/;

That's a very good example indeed, because regular expressions are mutable, so that meaning of const changes too.

const re = /a/g;
re.lastIndex; // 0
re.test('ba');
re.lastIndex; // 2

Do we really want to go down the rabbit hole of using let or cost for RegExp accordingly with the used flags? I hope no.

A bad example of const:

function updateElements()
{
  const elementCount = getNumberOfElements();
  for (let i = 0; i < elementCount; i++)
  {
    let element = getNextElement();
    doSomethingWithElement(element);
  }
  return elementCount;
}

That's perfectly valid and reasonable code. I would never nitpick on that. Actually, I would use const even inside the for but I couldn't care less because asking to change that would bring Zero benefit to me, the author of the code, and the project itself with a useless nitpick.

Now suppose we had to change the code to this:

We are software engineers, we change code all the time. I don't see this as a big issue. Code maintenance means, when needed, we change code. I am really easy going there.

I personally don't find it difficult to decide when to use const.

Neither do I, but if I have to explain to reviewers each time, it's counter productive for the company.

I am for enforcing rules to end these discussions.

If we have a rule able to enforce and understand what we want I'd be OK with that. But since it's per case, and have everything as let would be IMO just a stubborn solution so ... how can we improve the current situation?

Again, I have a review that never got a LGTM because I meant a const but I was told to use let, yet my linter was fine wiht me using const and me, as developer, really meant const (to me nodes are as "immutable" as regexpes, as example).

Few days ago another review (not mine) had similar discussion. It takes time to review everything we can do to speed up and focus on the right thing (logic, performance, algo) should be done.

Last edited on 04/16/2018 at 08:39:54 AM by agiammarchi

comment:16 follow-up: Changed on 04/16/2018 at 08:46:45 AM by agiammarchi

Last quick thought, if it could help anyone making a decision.

Always const is superior than always let because the majority of the time we want, mean, and need const (true for us as well as every project I've previously mentioned).

Check any code and see how many reassignment there are, usually in my experience so far, mostly every let means const.

Since we don't shadow references in block scopes, we create new let in there too, when we also mean const.

If the majority of the code assumes const, why are we using the wrong keyword?

Where are files where we reassign/change variables beside scalar values like an i in a for loop or that length example? Those are scalar, pretty valid candidate for const, accordingly to our rules, yet are the only references that need mutation across the code.

Last edited on 04/16/2018 at 08:55:07 AM by agiammarchi

comment:17 in reply to: ↑ 16 Changed on 04/16/2018 at 10:32:25 AM by saroyanm

Replying to agiammarchi:

Last quick thought, if it could help anyone making a decision.

Always const is superior than always let because the majority of the time we want, mean, and need const (true for us as well as every project I've previously mentioned).

That's what I do when I work on my projects(non eyeo related), it makes the code more readable IMO, but that can be a subjective opinion.

If we will enforce using const instead of let I'll be more happy, but I'm also okey using let if majority agrees, but we need to find proper wording for the coding style guide and ideally do linting if possible. Current wording Use const for constant expressions that could as well have been inlined do enforce using const in some cases, but doesn't disallow using it in some other places which still lead to some discussions example: for (const subscription of FilterStorage.subscriptions).

comment:18 Changed on 04/16/2018 at 10:33:59 AM by saroyanm

  • Cc juliandoucette ire added

comment:19 Changed on 04/16/2018 at 10:54:16 AM by agiammarchi

FWIW I am not sure everything let makes sense or there's even an ESLint rule for that.

For example, I would feel uncomfortable writing let BIG_CONST = 123;.

Having everything as let would also make us either care less about re-assigned references or, even worst, go overly paranoid about our own code, resulting in more debates instead of less.

I agree the wording needs to be improved, and I have initially suggested common sense but it's clear in here common sense about const and let is subjective too.

Ideally, whatever needs to be done, the overall goal should be less nitpicking to boost up reviews, focusing on the right thing.

let VS const, or vice-versa, where there are no side effects whatsoever in 3 lines of code is, in my opinion, a very good example on focusing on the wrong thing.

Having a linter saying who's right would make our code-reviews life easier.

That is prefer-const to me, but if we need to all agree on something let's make a poll based on the 3 options Sebastian mentioned?

Unless somebody else has other options.

comment:20 follow-up: Changed on 04/16/2018 at 11:02:12 AM by juliandoucette

(I don't really want to get into this unless you insist. FWIW I feel the same way as saroyanm in comment:17)

Last edited on 04/16/2018 at 11:02:57 AM by juliandoucette

comment:21 in reply to: ↑ 20 Changed on 04/16/2018 at 11:11:54 AM by saroyanm

Replying to juliandoucette:

(I don't really want to get into this unless you insist. FWIW I feel the same way as saroyanm in comment:17)

No, I intended to just CC you somehow, thinking that the discussion might be interesting for you to follow. Thanks for the feedback though.

comment:22 Changed on 04/16/2018 at 11:12:39 AM by greiner

Introducing "prefer-const" could be just as annoying as the "no-shadow" rule but it could also be just as helpful so no objections. My main concern, however, is how it affects existing code because we really don't want to break anything.

That being said, I'd be fine with any solution that is unambiguous because the way the coding style is phrased isn't.

comment:23 follow-up: Changed on 04/16/2018 at 01:55:19 PM by sebastian

To sum it up:

  • Everybody agrees that the current rule in our coding style guide is unclear, and should be improved.
  • Everybody agrees to use const for imports, aliases, identifiers and hard-coded values (i.e. those when changed over the course of the program's execution will inevitably break things).
  • Some people would favor to use const wherever possible, one person feels strong about it, and two disagree to that practice.

So it seems changing the rule in the coding style guide to my suggestion in comment:14, should be considered an improvement over the current situation at least by everyone, and probably the best we can do given that it seems unlikely that we will reach consensus to enforce const wherever possible.

comment:24 Changed on 04/16/2018 at 02:09:08 PM by agiammarchi

Some people would favor to use const wherever possible, one person feels strong about it, and two disagree to that practice.

Sorry Sebastian but this looks to me majority would prefer const, right?

Anyway, if there's no way to use prefer-const with even the help of a linter, I still argue about the meaning of real constants.

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.

If these are considered better and safer on top level, I don't understand why we would deliberately avoid such language feature in block scopes too.

In fact, if there is only one case were const is considered better, I don't understand any argument against const. If it's superior top level, it's superior block scope too and for the exact same reason: things are meant to be never reassigned and we want to protect that.

Couldn't we word it as such:

Use const for any reference which not being re-assigned isn't coincidental or matter to change in the future, but would inherently break things worth to protect against.

and move forward reducing nitpicking instead ?

comment:25 in reply to: ↑ 23 ; follow-up: Changed on 04/16/2018 at 02:10:16 PM by greiner

Thanks for the clear summary because it's difficult to catch up with everything that has been said.

Replying to sebastian:

So it seems changing the rule in the coding style guide to my suggestion in comment:14, should be considered an improvement over the current situation at least by everyone, and probably the best we can do given that it seems unlikely that we will reach consensus to enforce const wherever possible.

I don't agree with the conclusion because the suggestion is just as ambiguous as the existing rule. Based on the assumption that we can't fix the existing rule, the question is whether we want to keep the freedom to write code how we want it or to avoid such discussions in the future.

Based on your summary the issue appears to require the latter.

comment:26 in reply to: ↑ 25 ; follow-up: Changed on 04/16/2018 at 02:51:43 PM by sebastian

Replying to agiammarchi:

Sorry Sebastian but this looks to me majority would prefer const, right?

So what? This is not a poll. If we want to change something (in particular when there is no urgency) we have to find a consensus. If the topic is controversial, and we cannot resolve all disagreements, we have to find the largest common denominator we can agree on (or we just keep everything like it is).

Also FWIW, only two people from the WebExt team spoke so far, and both agree on more considerate usage of const. So the "majority" at the moment is quite biased towards UI.

Couldn't we word it as such:

Use const for any reference which not being re-assigned isn't coincidental or matter to change in the future, but would inherently break things worth to protect against.

and move forward reducing nitpicking instead ?

That sounds reasonable to me, but I wonder how sufficient it is to avoid further debates in code reviews.

Replying to greiner:

I don't agree with the conclusion because the suggestion is just as ambiguous as the existing rule. Based on the assumption that we can't fix the existing rule, the question is whether we want to keep the freedom to write code how we want it or to avoid such discussions in the future.

Based on your summary the issue appears to require the latter.

Well, relaxing our coding style guide would be an option too. But it seems what everybody feels most strong about is having a less (not more) ambiguous rule. Also I'd find it confusing to have a mix of using const whenever possible, and a more considerate usage of const, in the same code base.

comment:27 Changed on 04/16/2018 at 03:07:47 PM by kzar

  • Description modified (diff)
  • 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

... 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.

comment:28 Changed on 04/16/2018 at 03:09:57 PM by kzar

  • Component changed from Platform to Unknown

comment:29 in reply to: ↑ 26 Changed on 04/16/2018 at 03:26:26 PM by agiammarchi

Replying to sebastian:

So what? This is not a poll.

Fair enough. If anyone interested (it wouldn't be really fair to vote there though), I've actually published a twitter poll just to have even more opinions.

Well, relaxing our coding style guide would be an option too.

I am in favor of relaxing the style too. Common sense with reviewers effort to reduce nitpicking for this matter, since we all know it's easy to debate, could be a way forward.

However, I also agree with Thomas if the goal is to avoid anyone asking in the future why or when we do this or that, if there is a lint rule able to enforce a style, that'd really be it.

Let's remember most of the linter rules should be there to help us reduce most of the churn ... anyway ...

Another case against let

If we have top level const only, we might as well have live bindings in export. Imagine this file, it's called live.js.

// top level things, a lot of code ... then...
export let num = 1;

// another tons of lines of code ... then ...
export function many(i) {
  let mun = 0;
  while (i--) num++;
  return mun;
}

now we have an importer file, such app.js

import {num, many} from './live.js';

console.log(many(2)); // 0

console.log(num); // 2

What happened there? Even if both num and many has const like meaning, where no reassignment is possible, the exported num via let would 'cause a little disaster thanks to the typo inside many(...) export.

That's because the live binding will increase num by i times per each many(i) invoke.
If the same code gets transpiled as const {num, many} = require('./live'), such code might also break at distance without any warning.

The linter wouldn't care, maybe code reviewer didn't see the typo, and goodbye developers intent, logic, and code guards.

Some might think this example is stupid, but we have no-shadow names too and it might be easy to come up with similar references (think el and elem VS len and length or similar).

However, if we had prefer-const instead, nothing would've happened, and instead, an error would've thrown, 'cause the linter would've spot that before even a commit could land in any repository so that everything will be instantly obvious for the dev too.

And why wouldn't we desire to always code safer, when it's even easy to enforce it via a linter rule?

"If a feature is sometimes dangerous, and there is a better option, then always use the better option. " Douglas Crockford


@kzar thanks for updating the ticket.

Last edited on 04/16/2018 at 04:37:26 PM by agiammarchi

comment:30 follow-up: Changed on 04/16/2018 at 03:30:22 PM by sergz

Please take my apologizes for the absence of arguments.

If it were a poll I would vote for const everywhere unless let has to be used.

If there are real reasons for that (e.g. commonly acknowledged cognitive dissonance affecting the code quality, or a negative impact on the performance) then I would vote for const only for variables referencing primitives (they are already immutable) or manually enforced immutable objects (e.g. if someone comes up with some deepFreeze function). Nevertheless here IMO it is worth having a convention, e.g. capitalized constants.

I personally find Use const for constant expressions that could as well have been inlined (e.g. static parameters, imported modules, etc.). not clear enough and requiring an attention to stay in conformity. If we continue with that I would like to ask for examples (e.g. google style guidelines have them for most of the rules, I guess).

Whatever is decided, I don't think it makes sense to rush and change the whole codebase, thus enable any linting rule, at least there is already let which is almost the same as const in the runtime.

comment:31 in reply to: ↑ 30 Changed on 04/16/2018 at 03:40:35 PM by agiammarchi

Replying to sergz:

I don't think it makes sense to rush and change the whole codebase, thus enable any linting rule, at least there is already let which is almost the same as const in the runtime.

The --fix option would work pretty well here, either per file (incremental migration) or per project. Thomas AFAIK has similar concerns also to avoid breaking things (we shouldn't, IMO, actually we might discover bugs we didn't catch instead) but it doesn't have to be all at once.

If we agree on a way forward we know future code must follow that rule. To me that'd be already enough because I can hook my CTR+S automatic lint fixes and upgrade code as I go.

I also wouldn't need to revert/change 4 PRs because of my initial misunderstanding of the current style.

(edit: everything I've said actually only if we embrace prefer-const otherwise I will change my code, of course)

Last edited on 04/16/2018 at 03:41:44 PM by agiammarchi

comment:32 in reply to: ↑ 15 ; follow-ups: Changed on 04/18/2018 at 09:47:23 AM by mjethani

Replying to agiammarchi:

Replying to mjethani:

How can you tell the difference between variables that are intended to be const and those that are const only incidentally?

you can ask the same question about let

I don't think so. const is the exception, otherwise variables are supposed to be reassigned, that's why they are called "variables". C didn't even have the const keyword until "C with Classes" came along. If it's not a variable, it's a constant expression (think #define in C).

From that Wikipedia entry itself:

"It served two functions: as a way of defining a symbolic constant that obeys scope and type rules (that is, without using a macro) and as a way of deeming an object in memory immutable."

So const was supposed to refer to an immutable object in memory, otherwise #define would fine. JavaScript is confused about this (and I thank Java for this with its final keyword). I know you already said that we shouldn't look at other languages, but I'm merely pointing out that the way const works in JavaScript is really more like a #define in C, which works as if like copy and paste.

So how about this rule: "Use const only to refer to literals."

Valid:

const s = "string";
const n = 0;
const b = true;
const obj = {a: 1.0};
const arr = [1, 2, 3];
const re = /\<foo\>/;

Invalid:

const map = new Map([["a", 1], ["b", 2], ["c", 3]]);
const arr = new Array(10);
const error = new Error("Oh noes");

This rule is easy to follow. Everyone knows what a literal is, and literals would cover most of the cases where we want to use const.

It makes no sense to use const everywhere.

you can say the same about let

False equivalence. You can write code with only let, you can't write code with only const. let can be used instead of const, not the other way around.

[...]

If you cannot explain why you've declared something as const, you should leave it as let.

I don't want spend my time explaining when I want a guard, why would other devs?

In that case we should just use let everywhere. I personally haven't been in any discussion where the question of whether to use const or let came up, but since this issue is open I imagine it's a real problem. I'm OK with simplifying so we just use let everywhere.

A good example of const:

const elemhideRegExp = /^([^/*|@"!]*?)#([@?])?#(.+)$/;

That's a very good example indeed, because regular expressions are mutable, so that meaning of const changes too.

const re = /a/g;
re.lastIndex; // 0
re.test('ba');
re.lastIndex; // 2

Yes, but the example I used did not have the g option set, it is in fact immutable.

Do we really want to go down the rabbit hole of using let or cost for RegExp accordingly with the used flags? I hope no.

Based on my proposed rule above immutability doesn't matter, if it's a literal we just go ahead and use const (unless it has to be reassigned of course).

[...]

Now suppose we had to change the code to this:

We are software engineers, we change code all the time. I don't see this as a big issue. Code maintenance means, when needed, we change code. I am really easy going there.

Yeah, but my point was that the change would be unnecessary if it was let in the first place, it just creates an unnecessary line in the diff. It makes the change look more complex than it is. Like you have to think about why something went from being const to being let and what other effects it has (really it has no effect but why was it const then?).

I personally don't find it difficult to decide when to use const.

Neither do I, but if I have to explain to reviewers each time, it's counter productive for the company.

I am for enforcing rules to end these discussions.

[...]

Fair enough.

comment:33 follow-up: Changed on 04/18/2018 at 10:22:56 AM by mjethani

By the way, if it helps, in Chromium's C++ code there's no rule about when to and when not to use const. If you want to use const because that's what you mean, go ahead; if you cannot use const because you're mutating the object, you cannot use it, period. In any case there's no discussion about it if it's just a local variable. There are enough important things to discuss that no time is spent on such inconsequential things.

I feel like this is what we're doing wrong in the Adblock Plus project. We need to chill. If the developer wants to declare something as const and it's legal for it to be const, there's no point in nitpicking.

comment:34 in reply to: ↑ 32 Changed on 04/18/2018 at 10:28:32 AM by kzar

Replying to mjethani:

I personally haven't been in any discussion where the question of whether to use const or let came up...

FWIW I don't remember having any other discussions about let vs const since we came up with the current rules at the start of 2017.

comment:35 in reply to: ↑ 33 ; follow-up: Changed on 04/18/2018 at 10:34:54 AM by sergz

Replying to mjethani:

By the way, if it helps, in Chromium's C++ code there's no rule about when to and when not to use const.

There is, https://google.github.io/styleguide/jsguide.html#features-use-const-and-let

Declare all local variables with either const or let. Use const by default, unless a variable needs to be reassigned. The var keyword must not be used.

Update: sorry, misread it, there is Google JS style guide, the Chromium's C++ style guide is not relevant mainly because it's C++.

Last edited on 04/18/2018 at 10:36:29 AM by sergz

comment:36 Changed on 04/18/2018 at 10:45:47 AM by mjethani

To add some more background about const in JavaScript, it only works within scope.

There's no way to declare a class member as const, which is actually the main use case and why Java has it. I programmed in Java for many years and I don't remember anyone declaring a local variable as final. If you need to use the final keyword to prevent accidental modification to a local variable, you are doing something wrong. Either your function is too big (need to refactor) or you're just bad at writing code. Adding a final keyword to local variables unnecessarily is considered "too distracting".

In JavaScript if you want to guard a public member, you just have to make it a getter without a corresponding setter.

Properties exported from modules are already "const" from the outside, they cannot be reassigned.

// a.js
export let foo = 1;
// main.js
import {foo} from "./a";
foo++; // Error!

If you want to prevent modifications to your class members or module exports, const is not going to help.

So basically the main use cases for declaring something as a constant are not covered by the const keyword anyway, which is why I said it's a bells-and-whistles feature (and apparently a massive waste of time).

comment:37 in reply to: ↑ 35 Changed on 04/18/2018 at 10:59:10 AM by mjethani

Replying to sergz:

Replying to mjethani:

By the way, if it helps, in Chromium's C++ code there's no rule about when to and when not to use const.

There is, https://google.github.io/styleguide/jsguide.html#features-use-const-and-let

Yes, that's JavaScript. The problem appears unique to JavaScript somehow. For what it's worth it's not enforced so strictly (I used var here for example), consistency is more important.

comment:38 in reply to: ↑ 32 ; follow-up: Changed on 04/18/2018 at 12:28:46 PM by agiammarchi

Replying to mjethani:

JavaScript is confused about this

JavaScript follows ECMAScript standard and nobody at TC39 is confused about this, it's been spec'd as it is.

So how about this rule: "Use const only to refer to literals."

Your proposal makes little sense to me, specially when object literals are the one mutated the most, most frequently used as configuration object and/or mutated via Object.assign too.

Once again, const means immutable reference, that's the JavaScript meaning and we should embrace it when we write JavaScript, IMO.

False equivalence. You can write code with only let, you can't write code with only const. let can be used instead of const, not the other way around.

That's not true. When you import {a, b} from module those are constants, not let references. You cannot reassign them. That means you cannot technically consider every reference a let in modern JS.

Regardless, what I meant, is that it makes no sense to use always either one or another because const and let have different meanings and different use cases, not designed to be picked as the only way.

In that case we should just use let everywhere.

Wait a minute ... let me summarize the current status:

  • the majority of developers in this thread would prefer const
  • the majority of the internet would use const
  • the ESLint community is in favor of const and there is a rule to enforce that and put an end to debates (but no rule to enforce let 'cause you inevitably have const references in the code)

I'm afraid I miss the logic in your "then always let" conclusion.

I personally haven't been in any discussion where the question of whether to use const or let came up, but since this issue is open I imagine it's a real problem. I'm OK with simplifying so we just use let everywhere.

I personally wouldn't care less about these debates but like you said later on we have a nitpicking issue here. In this very specific case there is a rule that is ambiguous, so it's even easier to nitpick and debate about it.

If there was no rule, I would have happily ignored any nitpick about it. But if someone tells me (or others) I shouldn't have used const because of an ambiguous rule I interpreted in a way and the reviewer in another, we are all wasting both our time, and the time of the company we work for, for a change that will improve the code quality 0%, but its review iteration will double, and so will its shipping time.

Yes, but the example I used did not have the g option set, it is in fact immutable.

Not really. const a = /a/; a.lastIndex = 1; that's how far it goes RegExp immutability. The a.lastIndex will be 1.

Again, if we keep considering const what is not meant to be in JavaScript, we could write infinite examples of how well it fails, accordingly to other programming languages.

Look how wrong is Python where a = []; if a fails if a is an empty array instead of None ... right?
But how meaningless is, the point I've just made, if I write JS daily?

Should I know instead how truthy works in JS with or without a Python background? Oh hell yes I should!

Accordingly, if we understand what const means in JavaScript, we'll never have to discuss anything.
It means one thing only: immutable reference.

Any attempt to compare it with other languages and their meaning of const is useless because we write JS. If we'll change language we should rather write a migration from JS guide, not avoid all the new language things that in JS works differently.

Instead, here we are proposing to drop a language feature for no benefits whatsoever.

Based on my proposed rule above immutability doesn't matter

Immutability in JS rarely matters, but immutable references do matter: it's a guard, that comes for free, the language provides (inevitably via imports and/or exports too and to guard those).

Like you have to think about why something went from being const to being let

Not really, the developer that did that would explain or comment why, if necessary.
I think few lines in a for/of shouldn't cause any headache though.

and what other effects it has (really it has no effect but why was it const then?).

Tests with good coverage would prove there are no side effects. We should indeed focus way more on tests instead of these debates.
On top of that, 5 to 10 lines of code that changed const to let, where the need for that is immediately obvious, shouldn't be a big deal for junior to seniors devs.

Only changing a top level const to let would result in a very good example of case that needs explanation, but everything else probably wouldn't.

FWIW I don't remember having any other discussions about let vs const since we came up with the current rules at the start of 2017.

That demonstrates there is an issue indeed, because there is a rule. Developers that rightly want to stick with rules point at that.
Such rule is unfortunately ambiguous as it is, but we all want to move forward.

I feel like this is what we're doing wrong in the Adblock Plus project. We need to chill. If the developer wants to declare something as const and it's legal for it to be const, there's no point in nitpicking.

Amen that. So, how about we drop the second rule all together, and we keep the first one only?

Also considering Google JS code style

Declare all local variables with either const or let. Use const by default, unless a variable needs to be reassigned. The var keyword must not be used.

It looks like almost the entirety of the JS community out there prefers that way, so why shouldn't we follow that? Don't we collaborate to Open Source too? Don't we like to find a friendly repository where our daily code style just works in there too?

I personally do but here there is something I don't feel too comfortable about: it looks like we feel "better" than developers at Google, React, Angular, or all those using the ESLint prefer-const feature, which is agreed across many teams to be a sane, reasonable, compromise, for the meaning of const in the JavaScript language (which again is the language we use, and for which we need to understand how it works, including const).

I don't think we should start telling everyone else they are doing it wrong, including external contributors.
They will use const like the majority of the JS programming world and we'll have to say, every, single, time, to change to let and point at ... dunno, this discussion?

Or maybe we point at some January 2016 post where the author didn't appreciate the meaning of the (at time) recently introduced const, deciding always let was better, as substitute for var, even if these two have different purposes.

But from that post:

Constants are a feature of many languages.

It looks like he initially had the same comparison issue. He even stated:

For most variables, use let. For constants, use const. Constants should be declared at the top of modules and only in module scope.

Ad then, projects he contributes to use const for any sort of reference, 'cause that's how modern Ember style looks like too.

Maybe he changed his mind but never updated the initial post (or relaxed it in those dogmatic statements), who knows.

Last, but not least, I would like to underline that I don't want to change your personal habit neither.

I am all up for relaxing and reduce nitpicking about this.

That'd be lovely, and I think dropping the second rule as a whole, would also help this cause.

Then, if we're not happy and there is still nitpicking around this topic, we can think about enforcing via linter the rule like others out there do.

What do you say?

comment:39 in reply to: ↑ 38 ; follow-up: Changed on 04/18/2018 at 03:57:57 PM by mjethani

Replying to agiammarchi:

Replying to mjethani:

So how about this rule: "Use const only to refer to literals."

Your proposal makes little sense to me, specially when object literals are the one mutated the most, most frequently used as configuration object and/or mutated via Object.assign too.

Once again, const means immutable reference, that's the JavaScript meaning and we should embrace it when we write JavaScript, IMO.

I agree, so const is about immutable references, then why does it matter that the object it refers to can be mutated? I chose literals because it's unambiguous and covers most cases.

False equivalence. You can write code with only let, you can't write code with only const. let can be used instead of const, not the other way around.

That's not true. When you import {a, b} from module those are constants, not let references. You cannot reassign them. That means you cannot technically consider every reference a let in modern JS.

Regardless, what I meant, is that it makes no sense to use always either one or another because const and let have different meanings and different use cases, not designed to be picked as the only way.

I meant that you can write a perfectly fine program in JavaScript without a single use of const. Since it doesn't bring much value to the table, it's not worth using it if it comes at such a cost (i.e. lengthy discussions).

We have 3 options: (1) never use const (as if it doesn't exist); (2) use const whenever possible; (3) use const whenever it makes sense.

In that case we should just use let everywhere.

Wait a minute ... let me summarize the current status:

  • the majority of developers in this thread would prefer const
  • the majority of the internet would use const
  • the ESLint community is in favor of const and there is a rule to enforce that and put an end to debates (but no rule to enforce let 'cause you inevitably have const references in the code)

I'm afraid I miss the logic in your "then always let" conclusion.

Well, I don't mind going with whatever the majority prefers, I'm just expressing my opinion (count it as one vote if you will).

For what it's worth I'm not sure why we should care about the opinions of the "majority of the internet".

I personally haven't been in any discussion where the question of whether to use const or let came up, but since this issue is open I imagine it's a real problem. I'm OK with simplifying so we just use let everywhere.

I personally wouldn't care less about these debates but like you said later on we have a nitpicking issue here. In this very specific case there is a rule that is ambiguous, so it's even easier to nitpick and debate about it.

If there was no rule, I would have happily ignored any nitpick about it. But if someone tells me (or others) I shouldn't have used const because of an ambiguous rule I interpreted in a way and the reviewer in another, we are all wasting both our time, and the time of the company we work for, for a change that will improve the code quality 0%, but its review iteration will double, and so will its shipping time.

Yeah, I think we all agree that something needs to be done about that rule.

Yes, but the example I used did not have the g option set, it is in fact immutable.

Not really. const a = /a/; a.lastIndex = 1; that's how far it goes RegExp immutability. The a.lastIndex will be 1.

We don't modify that particular regular expression in that way, hence const makes sense in that particular case (but it's not going prevent the regular expression from being mutated, but nothing in JS is).

Again, if we keep considering const what is not meant to be in JavaScript, we could write infinite examples of how well it fails, accordingly to other programming languages.

Look how wrong is Python where a = []; if a fails if a is an empty array instead of None ... right?
But how meaningless is, the point I've just made, if I write JS daily?

Should I know instead how truthy works in JS with or without a Python background? Oh hell yes I should!

Accordingly, if we understand what const means in JavaScript, we'll never have to discuss anything.
It means one thing only: immutable reference.

Any attempt to compare it with other languages and their meaning of const is useless because we write JS. If we'll change language we should rather write a migration from JS guide, not avoid all the new language things that in JS works differently.

Instead, here we are proposing to drop a language feature for no benefits whatsoever.

So we both agree that const only means that the reference cannot be reassigned. I don't find that useful in ninety-five percent of the cases. We're talking about littering our code with const just for little benefit. I don't see the point. The only reason we're even talking about this is because ESLint could be made to complain about otherwise perfectly good code, just because the code doesn't adhere to one rule that most JavaScript developers seem to like. I'm OK if you think this is valuable but I think it's a total waste of time.

[...]

I am all up for relaxing and reduce nitpicking about this.

That'd be lovely, and I think dropping the second rule as a whole, would also help this cause.

Then, if we're not happy and there is still nitpicking around this topic, we can think about enforcing via linter the rule like others out there do.

What do you say?

My preferences in order (most to least): (1) never use const; (2) use const where it makes sense; (3) use const whenever possible. In the second case, we can drop the const-related rule in the style guide and go with "common sense" but I suspect the rule exists for a reason, I have my doubts if that is going to work.

Last edited on 04/18/2018 at 03:58:41 PM by mjethani

comment:40 in reply to: ↑ 39 Changed on 04/18/2018 at 04:24:22 PM by mjethani

Replying to mjethani:

So we both agree that const only means that the reference cannot be reassigned. I don't find that useful in ninety-five percent of the cases. We're talking about littering our code with const just for little benefit. I don't see the point. The only reason we're even talking about this is because ESLint could be made to complain about otherwise perfectly good code, just because the code doesn't adhere to one rule that most JavaScript developers seem to like. I'm OK if you think this is valuable but I think it's a total waste of time.

@agiammarchi or ... perhaps I'm misunderstanding?

What exactly is the benefit of declaring something as const in JavaScript? Can you give an example of where it is useful? I thought it was about conveying intent, but prefer-const clearly defeats that purpose.

Note that it's extremely rare that a variable is accidentally reassigned.

(You already gave a contrived example above, but that's never going to happen in Adblock Plus. We have code reviews for that.)

comment:41 follow-up: Changed on 04/19/2018 at 08:53:17 AM by agiammarchi

@mjethani again, the majority of the community prefers using little benefit over zero benefit.

const being meaningless and a waste of time is your personal opinion, ignored by the language itself, because imports are constants, so constants are there whenever you think are meaningful or not, and opposite to what the community thinks.

We also review code, of course, but I don't understand why you want to be sure no reference is ever reassigned by accident, when the JavaScript parser, and upfront a linter, could do that for you.

Re-quoting again Crockford:

"If a feature is sometimes dangerous, and there is a better option, then always use the better option. "

Accordingly, if there is a little benefit, and the code is safer/more guarded by machines, not just reviewer eyes, why would you ban such feature from the language?

At this point, everything that won't look like Java in the future ECMAScript, should be banned by our team, even if it's been stated for 20 years Java is to JavaScript what Ham is to Hamster.

There are also no real counter-arguments that demonstrate let is superior ... "we reviewing code won't ever make typos happen" also means we could drop the linter already, 'cause we're infallible, right?
But I'm sure you would disagree on that.

So, I wonder how come you want to introduce possible issues on purpose, trusting human eyes more than machines, demanding more attention to everyone, for zero benefits for the company.

I am proposing to embrace what every JS dev out there does already, and to be able to enforce it via tools we trust and use daily.

You are proposing to put more developers effort per each review to be sure things never get reassigned.

Please try to free your mind, as software engineer, and come back in 5 minutes to read the following sentence:

at ABP we decided to never use const, in JavaScript, because it has a different meaning in Java, PHP, C, or Turbo Pascal.

I'm afraid I don't see logic in this.


Anyway, since declaring references/variables is 80% of what we do daily as developers, how do we move forward here?

I don't have intention to counter-argument further because everything I wanted to say has been said, but this has become a blocker for UI team so, we need to find a solution ASAP.

  • should we vote? ('cause always let has no consensus)
  • should we drop the second point and move on with common sense to try and see if we are capable of that? (my fav option, it pleases everyone)
  • should we end this forever with the ESLint rule and call it a day? (the imperfect way that'll have inevitably best results in the long term)

Thanks for collaborating.

comment:42 follow-up: Changed on 05/18/2018 at 12:07:51 PM by juliandoucette

comment:43 in reply to: ↑ 42 ; follow-ups: Changed on 05/18/2018 at 12:59:10 PM by agiammarchi

Replying to juliandoucette:

Here is a relevant post and discussion about es6-const.

It looks like Mathias thinks same way most of us do as well and just to give everyone an update, since there was no progress here but still discussion within the UI team, we already adopted prefer-const ESLint directive and put an end to every possible related nitpicking within our team: https://gitlab.com/eyeo/adblockplus/adblockplusui/merge_requests/13

It's been a 20 minutes job between passing our code under eslint --fix and merge it in our master, and I suggest other teams in favor of const do the same.

Dropping that rule in the future is still possible without any side effect, but I wish we won't ever need to, and be consistent with the rest of the JS community and possible external contributors too.

comment:44 in reply to: ↑ 41 ; follow-up: Changed on 05/18/2018 at 07:34:49 PM by mjethani

Replying to agiammarchi:

@mjethani again, the majority of the community prefers using little benefit over zero benefit.

The benefit has to be weighed against the cost. I'll explain in my next comment why it is costly to use it.

About the "community," I'm not sure who or what you're referring to, but the only community I currently care about is the people who make contributions to Adblock Plus. If somebody is not helping Adblock Plus then I don't see why we should care about their opinion. Every project is different, and the standards that work best for one project may not work best for others.

const being meaningless and a waste of time is your personal opinion, ignored by the language itself, because imports are constants, so constants are there whenever you think are meaningful or not, and opposite to what the community thinks.

Once again you conflate the const keyword with constness itself.

If something that should be constant is constant by default (i.e. it comes for free), how could anyone be against that?

We also review code, of course, but I don't understand why you want to be sure no reference is ever reassigned by accident, when the JavaScript parser, and upfront a linter, could do that for you.

Because, like I said, it comes at a cost, and we must weigh the cost against the benefit. In the case of Adblock Plus, the benefit is zero because we don't "accidentally" modify code, we are very deliberate about it to begin with.

Re-quoting again Crockford:

"If a feature is sometimes dangerous, and there is a better option, then always use the better option. "

Accordingly, if there is a little benefit, and the code is safer/more guarded by machines, not just reviewer eyes, why would you ban such feature from the language?

I'm not against the use of the const keyword, I'm against the superfluous (and misleading and frankly idiotic and counterproductive) use of it. I'll explain in my next comment what I mean by this.

At this point, everything that won't look like Java in the future ECMAScript, should be banned by our team, even if it's been stated for 20 years Java is to JavaScript what Ham is to Hamster.

Sorry, this is not about Java.

I may use examples from Java and C++ to make my case because JavaScript doesn't have any such examples as the const keyword where it's so easy to overuse and misuse something "because you can." Java and C++ have plenty of such features (and decades of history), as I'll explain in my next comment.

There are also no real counter-arguments that demonstrate let is superior ... "we reviewing code won't ever make typos happen" also means we could drop the linter already, 'cause we're infallible, right?
But I'm sure you would disagree on that.

If it helps, imagine that instead of const vs. let this was about const let vs. let. const is an additional constraint on a variable (a "variable" by definition is modifiable, unless you place a constraint on it). The question then is when to place this constraint on a variable.

So, I wonder how come you want to introduce possible issues on purpose, trusting human eyes more than machines, demanding more attention to everyone, for zero benefits for the company.

Can you point me to a single example in Adblock Plus where an "issue" happened because of a reference being accidentally reassigned that would not have happened if we had used the const keyword?

I'm also still waiting for an answer to my very simple question: What exactly is the benefit of declaring something as const in JavaScript? Can you give an example of where it is useful?

I am proposing to embrace what every JS dev out there does already, and to be able to enforce it via tools we trust and use daily.

I would really prefer if we dropped the "every JS dev" thing in this discussion. Why not "every dev" or "every human" or "every living organism"? If we want to do anything based on the lowest common denominator, we can go a long way.

You are proposing to put more developers effort per each review to be sure things never get reassigned.

There is no additional effort in making sure that nothing is accidentally reassigned. It's all part of the effort of making sure a patch does what it should do, nothing more, nothing less. I suggest you spend some time actually reviewing other developers' patches to see the kinds of issues that come up.

Please try to free your mind, as software engineer, and come back in 5 minutes to read the following sentence:

at ABP we decided to never use const, in JavaScript, because it has a different meaning in Java, PHP, C, or Turbo Pascal.

Well, that statement is false, so there's nothing to reflect on.

I would prefer that we never use the const keyword, especially not prefer-const, because it's noise. I would prefer our code to have a high signal-to-noise ratio. When you look at code, it should tell you what's relevant and not tell you what's irrelevant. The const keyword has almost zero benefit and is mostly a distraction.

Anyway, since declaring references/variables is 80% of what we do daily as developers, how do we move forward here?

I'm not sure, but we could start by either never using the const keyword or by exercising our own judgement based on the individual case.

I for one have way more important things to do than to make sure that variables are not accidentally reassigned by a developer we interviewed, hired, and are paying a decent salary to write good, solid code that is also thoroughly reviewed for lots of other actually important things.

comment:45 in reply to: ↑ 43 Changed on 05/18/2018 at 07:39:56 PM by mjethani

Replying to agiammarchi:

Replying to juliandoucette:

Here is a relevant post and discussion about es6-const.

It looks like Mathias thinks same way most of us do as well and just to give everyone an update, since there was no progress here but still discussion within the UI team, we already adopted prefer-const ESLint directive and put an end to every possible related nitpicking within our team: https://gitlab.com/eyeo/adblockplus/adblockplusui/merge_requests/13

Interesting, I have nothing against individual modules choosing whatever approach works best for them.

I'll try to keep prefer-const out of the Core module if I can (depends on others' opinions). So far in Core we're using "common sense" and it's working out quite OK for us, in my opinion (but others please correct me if I'm wrong).

comment:46 in reply to: ↑ 44 ; follow-up: Changed on 05/18/2018 at 11:51:40 PM by agiammarchi

Replying to mjethani:

I would prefer that we never use the const keyword, especially not prefer-const, because it's noise.

To be fair, the only noise I see is this weekly standing debate with no consensus.

In ABP UI team we put an end to all this noise and moved on after a 20 minutes job.
That's time and money saved, also an end to every possible future discussion.

The const keyword has almost zero benefit and is mostly a distraction.

Almost > 0, which logically is already a good hint, but regardless, to me the worst distraction ever is useless nitpicking, which is also very demotivating.

Like you said, we all have better things to do and prefer-const is the only rule we have in the linting tool we've chosen.

So, to put an end to these discussions, we (UI) preferred to move on and use what the tool provides, and it's working without glitches and it has zero side-effects.

we could start by either never using the const keyword or by exercising our own judgement based on the individual case.

Your judgment is clearly different from mine so I prefer to have a referee (ESLint) decide and never ever wast anyone time on this again in the future.

I for one have way more important things to do than to make sure that variables are not accidentally reassigned

That is exactly why we use a linter, to avoid being distracted by errors the linter can catch already.

If you, for one, have better things to do, why do you ever want to focus on references that might accidentally be reassigned?

a developer we interviewed, hired, and are paying a decent salary to write good, solid code that is also thoroughly reviewed for lots of other actually important things.

And you want to waste time reading every reference to be sure that won't ever be re-assigned?

With the decent salary we are paid for, we should aim to the least effort for everyone to review and quickly move forward, focusing on the right thing, that is never a de-facto zero-benefits coding style rule that any linter could verify for us (if there's an option to do so, of course).

Accordingly, In ABP UI this worked already, and I humbly suggest other teams do the same.

So far in Core we're using "common sense" and it's working out quite OK for us

I had again the same "should it be a const or let" discussion few days ago and in few minutes we agreed on putting an end to this debate bringing in a natural, battle tested, native JavaScript guard that is const via ESLint prefer-const and it worked.

We started the discussion in IRC and we landed the refactoring, with approval, and a merge in master, in a total amount of 2 hours: problem solved!

I also personally wouldn't be too stubborn about avoiding const, because it has reasons to exists and pretty much everyone in the field is OK with it.

But regardless, if we care only about the ABP community here, for what I can read, the majority in this ticket is in favor of const too.

But of course, you are 100% free to avoid const or judge every single usage of it per each code review, but FWIW, in the UI team case, the prefer-const instantly worked pretty well and, best of all, it took less than writing these answers to put an end to such debate, which is the reason I keep suggesting other teams to do the same.

As summary: when common sense doesn't work ('cause it's not common, as this ticket demonstrates), you gotta be pragmatic!

Last, but not least, since I have no intention to keep discussing this ticket, or const vs let in general, if everyone is OK with it, I can close it so we can all move on to those better things to do.

Best Regards.

comment:47 in reply to: ↑ 46 ; follow-up: Changed on 05/19/2018 at 03:28:46 AM by mjethani

Replying to agiammarchi:

Replying to mjethani:
[...]

In ABP UI team we put an end to all this noise and moved on after a 20 minutes job.
That's time and money saved, also an end to every possible future discussion.

I admire your optimism and I hope it works out.

The const keyword has almost zero benefit and is mostly a distraction.

Almost > 0, which logically is already a good hint, but regardless, to me the worst distraction ever is useless nitpicking, which is also very demotivating.

After deducting the cost it's < 0 and that's what I meant, as I indicated in the opening sentence.

Like you said, we all have better things to do and prefer-const is the only rule we have in the linting tool we've chosen.

So, to put an end to these discussions, we (UI) preferred to move on and use what the tool provides, and it's working without glitches and it has zero side-effects.

Well my only concern is that it'll make it harder to rebase because of the extra lines in the diff changing const to let, but it'll be interesting to see how this works in practice.

Fingers crossed!

(As for prefer-const being the only option, I'll try to implement a no-const patch on the weekend and submit it to ESLint. prefer-const is just too profoundly stupid for it to be the only option for teams of JavaScript developers that keep having this debate for some reason.)

[...]

I for one have way more important things to do than to make sure that variables are not accidentally reassigned

That is exactly why we use a linter, to avoid being distracted by errors the linter can catch already.

If you, for one, have better things to do, why do you ever want to focus on references that might accidentally be reassigned?

I don't get this question. I literally don't care if a reference is assigned once or multiple times (I have maintained this throughout this discussion), except in the extremely rare cases in which it does matter. In those extremely rare cases I'm in favor of using const, in the vast majority of cases where the number of assignments to a reference is of no significance I favor using let.

a developer we interviewed, hired, and are paying a decent salary to write good, solid code that is also thoroughly reviewed for lots of other actually important things.

And you want to waste time reading every reference to be sure that won't ever be re-assigned?

No, I don't. Nobody does. We don't check if a reference is assigned once or multiple times. We don't care. It is of no significance. It never comes up. It is never discussed. It truly, truly, truly doesn't matter.

Which is why I don't see any use of const.

[...]

But regardless, if we care only about the ABP community here, for what I can read, the majority in this ticket is in favor of const too.

The majority already chose to go with prefer-const in the code that they mainly work on. I'd say let's wait and see how that works out.

But of course, you are 100% free to avoid const or judge every single usage of it per each code review, but FWIW, in the UI team case, the prefer-const instantly worked pretty well and, best of all, it took less than writing these answers to put an end to such debate, which is the reason I keep suggesting other teams to do the same.

We don't have this problem. I have no idea why this keeps coming up in UI.

[...]

Last, but not least, since I have no intention to keep discussing this ticket, or const vs let in general, if everyone is OK with it, I can close it so we can all move on to those better things to do.

If the issue was about clarifying the rule about const usage in our style guide, I think that question is still open, but perhaps it is of less significance now since at least one module has decided to try out one way of doing it.

You might close it and if anyone else is not satisfied with the current state of things then they might reopen it.

Last edited on 05/19/2018 at 03:32:31 AM by mjethani

comment:48 Changed on 05/19/2018 at 08:41:59 AM by agiammarchi

  • Resolution set to worksforme
  • Status changed from new to closed

comment:49 in reply to: ↑ 47 Changed on 05/19/2018 at 08:51:42 AM by agiammarchi

Replying to mjethani:

As for prefer-const being the only option, I'll try to implement a no-const patch on the weekend and submit it to ESLint.

That'd be great so you can also have a response from the community, not only eyeo developers.

Yet, since we're already happy in UI, we'd like to keep being happy with prefer-const and forget about this topic for the time being.

prefer-const is just too profoundly stupid for it to be the only option for teams of JavaScript developers that keep having this debate for some reason.

Everyone that adopet that profoundly stupid rule inevitably stopped debating about const vs let.

There's also to consider that if there was another rule, there would be to debate about which one is better so, to some extend, I am very happy there is only one rule to put an end to these debates.


I've closed the ticket as "worksforme" since that's basically what UI did: overwriting the generic linting rule after refactoring via eslint --fix the code to adopt prefer-const rule in our UI code base.

If anyone wants to keep this open or re-open it, I have no objections, but please, if possible, don't involve me in the future with this thread 'cause we, as a team, made a decision and moved happily forward already.

Thanks for your understanding.

Best Regards.

Last edited on 05/19/2018 at 08:52:42 AM by agiammarchi

comment:50 in reply to: ↑ 43 ; follow-up: Changed on 05/22/2018 at 03:06:35 AM by mjethani

Replying to agiammarchi:

Replying to juliandoucette:

Here is a relevant post and discussion about es6-const.

It looks like Mathias thinks same way most of us do as well [...]

I have posted a comment there, but it might be a while before it appears (or perhaps never), so here's a copy:

https://gist.github.com/mjethani/a6861e3601ec883c5cc6dc7f226c07e7

comment:51 in reply to: ↑ 50 Changed on 05/22/2018 at 10:30:14 AM by agiammarchi

Replying to mjethani:

I have posted a comment there, but it might be a while before it appears (or perhaps never), so here's a copy:
https://gist.github.com/mjethani/a6861e3601ec883c5cc6dc7f226c07e7

I am glad to know you consider me a dumb zoo monkey that has no idea what is doing, but like I've said, the rule simply puts an ends to all of this.

Discussing with such attitude is also something I am not willing to do.

Following what I think you might consider improving when arguing about something.

one of the dumbest things (implies everyone is dumb)
I'm embarrassed I have to do this (implies you are better than everyone else)
you're a professional JavaScript developer (passive aggressive until the end of the sentence)
I'm sorry, this too embarrassing. I'll stop. (good because the example is not even meaningful)
On being presented with a tool that one has no idea how to use (implies others don't know what they are doing)
JavaScript developers these days are like zoo monkeys (now you are insulting JS developers)
If you don't know how to use something, just don't use it; it's that simple. (implies only you know how to use a linter)
Nevertheless, I must say that in my day job I write software that runs on a hundred million devices, nonstop (so do others, specially at Google, and so do I, before eyeo and in eyeo too, we all survived using const "the dumb way")
the so-called "JavaScript community" which might just be a handful of influential developers with too much time on hand (you just wrote a huge rant too, I am sure it took time)
ESLint's prefer-const rule and the "philosophy" behind it is only a final manifestation of a long descent into mass insanity (passive aggressive)
I wish I had the time to pick apart every one of their silly practices and the meaningless rituals (passive aggressive and arrogant)

Anyway, I'd be happy in the future to eventually consider with the rest of the team your contribution to ESLint with better heuristics for const and let.

Best Regards

Last edited on 05/22/2018 at 01:54:31 PM by agiammarchi

Add Comment

Modify Ticket

Change Properties
Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from (none).
 
Note: See TracTickets for help on using tickets.