search

firefox-devtools / rfcs

RFCs for changes to DevTools
15 stars 17 forks source link

Discussion: use of let and const #44

Closed codehag closed 6 years ago

codehag commented 6 years ago

This is something that I noticed in our codebase on mozilla central, but not in our github codebases - the use of var, let and constis pretty inconsistent. Linting is also turned off for a number of files so we don't even get errors for using var half the time. We should turn on the linting, but there remains the issue of let and cost which are currently used somewhat without clear intention.

My thought is we should use let and const strictly, as it reduces the mental load when reading code. If let is always used, you have to watch for reassignments, and it makes it easy to miss potential problem areas. On the other hand, using const wherever possible, and let only when something is going to be reassigned, you can pay less attention to this rather tedious part of code reading.

In this same vein, I would recommend to have a linting rule that does not allow modification of arguments.

gregtatum commented 6 years ago

It takes more effort for me to read code that is not strict about const and let. My preference would be to enable the eslint rule for the stricter mode. I find that there are very few instances where let is need, and the vast majority of my assignments are const. It makes it easier for me to reason about the code, and is something that eslint --fix can do automatically.

juliandescottes commented 6 years ago

Will make code reviews easier, in favor of enforcing this via linting.

I guess the rule for this is prefer-const: "error" https://eslint.org/docs/rules/prefer-const ? mozilla-central is not defining anything for this. Our code would still be compatible with mozilla-central rules, just a bit stricter. @Standard8 any concern on your side?

I would recommend to have a linting rule that does not allow modification of arguments.

Somehow I still like to set defaults via arg = arg || default; rather than having everything in the signature. Would be interested to see our current violations for this rule. Could we move that to a separate RFC?

MikeRatcliffe commented 6 years ago

I am completely for using const everywhere unless we need to use let.

janodvarko commented 6 years ago

WebExtensions API code base is already using const everywhere. See e.g. devtools.panels related code base: https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/browser/components/extensions/child/ext-devtools-panels.js#249

So, I am also for using const and force it via linting.

Honza

Standard8 commented 6 years ago

I think this is generally a reasonable thing to do. I think if the WebExtensions code is using it already, then it might be worth a discussion on dev.platform/firefox-dev - to see if we could adopt it globally, and check there's no hard-blockers/issues (which I really doubt).

Devtools could still adopt it anyway (and we could e.g. do the devtools directory first), but if we can plan to adopt it globally, then we could keep down the differences between devtools & the rest of the code base.

(Personally when I found this style a few weeks ago I didn't like it as const seemed a bit long and it seemed harder to read. However, I think that's initial-look "shock", and I'd easily get used to it).

juliandescottes commented 6 years ago

Reviewed this during this week's RFC meeting. All the feedback so far was very positive. Since the RFC was only opened 2 days ago we will give it some more time for comments, and will take a final decision next week.

Most likely the actions that would follow if this is accepted are:

Quick note about the code in WebExtensions. const is used more consistently than in DevTools codebase, but it is not checked by linting. So far the rule prefer-const is not used anywhere in mozilla-central, so DevTools will be the only user if we introduce it.

julienw commented 6 years ago

As a quick note: I don't really mind one way or the other. The eslint rule prefer-const makes this easy to implement. More and more I'm using a code style where I use const nearly everywhere, like Greg already mentioned.

But for the sake of having counter-arguments I wanted to share this piece of advice: https://jamie.build/const (careful: contains offensive language). I think this post makes very good points, I'll try to summarize them here without the offensive language:

  1. const doesn't prevent much: it doesn't prevent mutating properties especially.
  2. linters doesn't help with this either
  3. JS engines don't do anything special with const -- they know better than you already
  4. communicate something doesn't change -- but what we do when we automate using the eslint rule prefer-const is that we use const if, for some reason, this binding isn't reassigned. So we get the opposite result indeed: the causal direction is inverted, because we use const because the binding isn't reassigned, instead of not assigning the binding because const.
  5. const is useful at the top level and we should never use let at the top level because this means we have a global state.

Point 4 resonates with me actually. It happened frequently that while I'm changing some code the linter changes my let into a const just because the code isn't finished yet. Also sometimes some variable should be conceptually const but I have to use let because I have to assign a value depending on some conditions, can't use a ternary at that moment, and Javascript doesn't (yet) have blocks returning values like Rust ;-) Example:

let variable = default;
if (someCondition) {
  // do something that needs more than 1 call;
  variable = resultOfComputation;
}

About point 1: if we use immutability more (but we can't enforce it), this makes more sense.

So basically: my personal taste is the same as others here. Yet when I think of it I feel this is "just" a matter of taste and doesn't bring anything more.

MikeRatcliffe commented 6 years ago

So basically: my personal taste is the same as others here. Yet when I think of it I feel this is "just" a matter of taste and doesn't bring anything more.

As far as I understand, one advantage is improved type specialization. Using const with primitives helps preserve the data type, which gives us tiny optimizations when jitting... we are only talking micro optimizations though.

juliandescottes commented 6 years ago

Nobody strongly opposed the RFC. So the proposal was accepted with the following actions proposed:

@codehag would you like to create the bug ?

Additional clarification: prefer-const is only about let vs const, which means that var is out of scope of this RFC. I assume there's consensus that we shouldn't use var, but replacing it is more tricky (not sure if it needs a RFC or just some polish/debt bugs).

codehag commented 6 years ago

bug is here! https://bugzilla.mozilla.org/show_bug.cgi?id=1454696

juliandescottes commented 6 years ago

Great! Thanks @codehag closing this one.

juliandescottes commented 6 years ago

(I was sure I added the accepted label last week :( sorry! )