Discussion on rules for const and let

[enjikaka]

We now have one-var as a rule which:

Enforces variables to be declared either together or separately per function (for var) or block (for let and const) scope.

Today this is confed like this;

• "const": "never" - requires multiple const declarations per block
• "let": "always" - requires one let declaration per block
• "var": "always" - requires one var declaration per function

let has inherited the settings of var. Some does not fancy this though.

This issue will serve as a thread in which we discuss if we should change the rule for let.

--

"let": "always" enforces:

let something = 1
  , somethingElse = 2
  ;

"let": "never" enforces:

let something = 1;
let somethingElse = 2;

see http://eslint.org/docs/rules/one-var for more.

[eirslett]

I suggest that we change the configuration to "one-var: never", in other words, use one keyword per declaration.

Reasons why:

Smart peple have already written about it

one-var comes from the var era

var does function scope hoisting, which is regarded by many as a flaw in the design of the JavaScript language. That's maybe why let was introduced; to mitigate the problems introduced with hoisting (since let is not hoisted).

function foo () {
  var a
    , b
    , c
  ;
  doStuff();
  c = 1;
  var d = 2; // weird because d is hoisted to the top of the function, so it would be better to declare it there as well
}

When writing ES 5 code, it makes sense to follow the conventions of one-var and vars-on-top, because that aligns the code more closely with what JavaScript does with the code at runtime.

Since we now have let and const, that argument is no longer valid.

one-var stops us from being able to define variables where they are used

Hafsteinn:

my only issue is the defining everything at the top before assigning them values. I find code more readable when you assign variables where you use them... gives everything a little more context.

Jeremy:

Like Hafsteinn Ingason I agree we should put the const and lets where they are defined.

I agree with this.

"one-var: always" limits us. Consider this code:

function myFunc () {
  let a, b, c;
  doStuff();
  doMoreStuff();
  doEvenMoreStuff(a, b, c);

  // declare variables where they are used
  let d, e, f;
  doSomeStuff(d, e, f);
}

It will not allowed, because of the one-var rule. We have to define d, e and f in the top, which is not where they are used.

Do what other developers do

It's nice to be a special snowflake in a boring world, but in technology it often makes sense to make the same decisions as everybody else, and be snowflakey in product decisions instead of in code style decisions.

Random examples: npm uses standardjs, and looks like this (random file example):

var Conf = require('../config/core.js').Conf
var CachingRegClient = require('../cache/caching-client.js')
var log = require('npmlog')

module.exports = getPublishConfig

function getPublishConfig (publishConfig, defaultConfig, defaultClient) {
  var config = defaultConfig
  var client = defaultClient
  log.verbose('getPublishConfig', publishConfig)
  // ...

notice one var keyword per declaration.

eslint-config-xo, which eslint-config-tidal is based on, uses one-var: ['error', 'never']. We actually override XO to set our own config for one-var.

The Airbnb style guide is used a lot in other projects, and it mandates one keyword per declaration. ("one-var": ["error", "never"])

Google has good JavaScript engineers, their style guide also mandates separate keywords.

Same with Walmart and other companies that have made their eslint setup public.

To be frank, I haven't seen one single eslint config out there that uses "one-var: always". Do you know of any? (Except TIDAL of course)

git diffs and merge conflicts

When we add or remove variables, we can get strange diffs:

var a = 1,
  b = 2,
  c = aLongAndComplicatedExpression(1, a, b, doStuff());

Adding another variable on the bottom means we must replace the semicolon with a comma. Same if we remove c, we must replace the comma in line 2 with a semicolon, so we get an unnecessary diff.

The common way to mitigate that, is to write syntax like this:

var a = 1
  , b = 2
  , c = xxx
  ;

but it's not very elegant, compared to the simple

var a = 1;
var b = 2;
var c = xxx;

where things can be added and removed without affecting neighbour lines.

Don't mix code styles

It's possible to mix, by using "always" for var and let, and "never" for const. But that is confusing. Why not just be consistent?

Save lines of code

let a
  , b
  , c
  , d
  ;

  doStuff();

  a = 1;
  b = 2;
  c = 3;
  d = 4;

  doMore(a, b, c, d);
  // do more stuff

versus

  doStuff();

  let a = 1;
  let b = 2;
  let c = 3;
  let d = 4;

  doMore(a, b, c, d);
  // do more stuff

where we saved lots of lines.

We really shouldn't be using let at all

We should be using reactive/functional/immutable paradigms when we build our software. Which mandates using const everywhere. Reverting to let should be a last resort (when you, for some reason, really need a mutable variable. For performance maybe, in a render loop?).

---

Other concerns:

Jeremy:

I think we should enforce sorting the different variable types

I too think it makes sense in some places to define lets first (for example) and then consts, and not mix them in one code chunk. I'm not sure if there's an eslint rule to enforce it, however. We can "enforce" it manually, by cleaning up the most obvious places when we encounter them?

[enjikaka]

Good points! I agree that forcing lets to be limited to 1 cluster in its scope is bad. But I do like the idea of making a cluser in the place they are defined, in the cases where many lets are defined at the same time.

This for example:

function boo () {
   const foo = 'bar';

   alert(foo);

   let imageUrl = 'http://img.website.com/path/to/img.png';
   let imageSize = 250;

   console.debug(imageUrl, imageSize);
}

Here I think something like this should be allowed (but not forced!):

function boo () {
   const foo = 'bar';

   alert(foo);

   let imageUrl = 'http://img.website.com/path/to/img.png'
     , imageSize = 250;

   console.debug(imageUrl, imageSize);
}

Also on this;

We should be using reactive/functional/immutable paradigms when we build our software. Which mandates using const everywhere. Reverting to let should be a last resort

I don't really agree here. While I do like to use const as much as possible, I think it really does not make sense when doing multiple transforms to a collection.

Take this for example;

function doAwesomeStuff (array) {
   const filteredArray = array.filter(/* some filtering */);
   const transformedArray = filteredArray.map(/* some transforming */);
   const finalValue = transformedArray.reduce(/* some reducing */);

   return finalValue;
}

I think that looks really ugly, creating 3 variables to be able to do what I need with the collection. And imagine if more tweaking to the collection was needed! The only option, while still using const here, or not a variable at all and returning directly, is introducing a callback hell like this;

function doAwesomeStuff (array) {
   return array.filter(/* some filtering */)
                       .map(/* some transforming */)
                       .reduce(/* some reducing */);
}

Imagine all the filter, map and reduce methods being a bit too big, and doesn't make sense to move out of the function scope into own methods. That'd make the code very unreadable. In that case I'd prefer this let version;

function doAwesomeStuff (array) {
   let modifiedArray = array.filter(/* some filtering */);

   modifiedArray = modifiedArray.map(/* some transforming */);
   modifiedArray = modifiedArray.reduce(/* some reducing */);

   return modifiedArray;
}

[cytrowski]

In case "some filtering" being too long we can always use named functions there and move them where they can be reused. Usually when we need more than few let declarations it's a sign of code smell - the code may be doing more that it should. I see nothing wrong with having multiple const declarations for partial filtering / mapping result.

For me having multiple let definitions is superior to be forced to do the "comma-first" festival - it lefts no place for "I really want to have those commas here" discussions ;)

[eirslett]

The "don't use let" stuff is a pretty big detour from this code style topic (sorry I even brought it up, I shouldn't have)

I agree that in some places, using let can be tempting, like the array example. But sometimes it's useful to think of variable declarations as math equations; when you change the right side you have to change the left side too, correspondingly.

Here is a silly (strawman) example:

let britishArtists = [{name: 'Elton John'}, {name: 'Coldplay'}];
let americanArtists = [{name: 'Madonna'}, {name: 'Britney Spears'}];
britishArtists = [...britishArtists, ...americanArtists];
britishArtists = britishArtists.map(artist => artist.name);
console.log(britishArtists);

Here, we change the right side, without changing the left side (we inappropriately reuse the britishArtists variable), so there is a mismatch between what the variable says it's supposed to contain (data about british artists), and what it actually contains (names of artists of multiple nationalities).

The easy way out of that, is to use intentionally ambiguous names like "item", "object", "model", "collection", "array" etc., to hide the fact that you change the right side without changing the left side. But intentionally using ambiguous variable names is not good practice (if you ask me).

Personally I prefer consts with detailed names:

const britishArtists = [{name: 'Elton John'}, {name: 'Coldplay'}];
const americanArtists = [{name: 'Madonna'}, {name: 'Britney Spears'}];
const allArtists = [...britishArtists, ...americanArtists];
const allArtistNames = allArtists.map(artist => artist.name);
console.log(allArtistNames);

The functional/immutable school of thought is that once you have applied a transformation on a collection, you get a new collection, which is separate from the old one, so setting up separate consts like that makes sense.

[eirslett]

Looks to me like multiple developers on the team (most of the people who took part in the discussion at least) want to move to "one-var": ["error", "never"] (which means one var/let/const keyword for each variable declared). I don't think we will get any more responses on this issue. Should we go ahead then, maybe?

[enjikaka]

Øyvind has the final say on code style afaik, so not until he comments on this.

(I'm all for the one-var change though)

[enjikaka]

Woops, clicked the wrong button haha. #GoodGitHubUX

[cytrowski]

For me the function arguments should be treated as consts. We never modify them.

[enjikaka]

But sometimes it's useful to think of variable declarations as math equations; when you change the right side you have to change the left side too, correspondingly. /.../ The functional/immutable school of thought is that once you have applied a transformation on a collection, you get a new collection, which is separate from the old one, so setting up separate consts like that makes sense.

I don't agree that const is superior and the functional/immutable school of thought fitting in most cases. Transforming a collection many times via const will eventually make the variable names be totally irrelevant to what they contain, or much less descriptive. Sure, many of the schoolbook examples look nice. But in the real world they end up giving less legible code.

Take this method from your imageUrlHelper refactoring for example;

getImageUrl (resourceId, w, h, type) {
    const id = resourceId || fallbackImages[type];
    if (id) {
      const size = [w, h];
      const idPath = id.replace(/-/g, '/');
      validateImageSize(size, type);
      const adjustedSize = adjustForRetinaSupport(size, type);

      return imageUrl
        .replace('#picture#', idPath)
        .replace('#size#', adjustedSize.join('x'));
    } else {
      return null;
    }
}

Here we the descriptive resourceId immediately gets "renamed" to the less descriptive id, only because you wanted to use a const? (not a valid reason imho) Can you come up with a better name that doesn't make the variable name less descriptive or the variable name very much more redundant? I can't. The const idPath = id.replace(/-/g, '/'); makes perfect sense I think though. Since we actually transform that to a path :)

[enjikaka]

For me the function arguments should be treated as consts. We never modify them. ~ @cytrowski

That wouldn't work out that often? We often need to verify the arguments are correct within the method. And if you take the getImageUrl method above, I don't think getImageUrl (resourceId = fallbackImages[type], w, h, type) { would be possible for example?

[cytrowski]

It would ;)

const getImageUrl = (resourceId, w, h, type) => {
      const idPath = getIdPath(resourceId, type);
      return idPath ? 
          buildImageUrl(
              idPath, 
              adjustForRetinaSupport(
                  validateImageSize, [w, h], type
              )
          ) : null
}

With simple currying and responsibility decomposition it can be done always :)

[enjikaka]

@cytrowski But in that example legibility suffers from a too long ternary. Legibility should always be highest priority. Code is for humans to read. :)

[osmestad]

So to conclude: We currently enforce multiple 'const', we disallow 'var' and the question is if we should allow, force or disallow multiple 'let'. To keep in line with the rule for 'const' I think it is required to allow multiple 'let's, as it does seems strange to hoist one variable type only (seems to be consensus on wanting to declare as close to usage as possible). The other question, if it should be allowed to do multiple 'let' definitions in block style, (meaning dropping one-var validation rule for 'let', I assume we might need to drop that rule altogether for that to work), I am not sure if it is needed as I guess it will not be used that often anyway? One option might be to enforce clustering of uninitialized variables? "uninitialized": "always" and "initialized": "never". But to be honest maybe the simplest solution ('one-var': ['error', 'never']) might be good enough :)

[enjikaka]

I think this might be the best solution then;

'one-var': ['error', {
  "initialized": "never",
  "uninitialized": "always"
}]

[eirslett]

'one-var': ['error', {
  "initialized": "never",
  "uninitialized": "always"
}]

We could do that, correct code style will then be like this:

function foo() {
    let a, b, c;
    const foo = true;
    const bar = false;
}

But I don't think it's a good idea for our code base.

For the sake of correctness, it's usually a good idea to combine declaration and assignment in the same statement, because then you're guaranteed that the variable has a "valid" value before you use it:

// bad, because of declaration before assignment
let a;
doStuff(a); // fails with (possibly) strange error message about something being undefined
a = 1;

// good, because declaration and giving it a valid value is combined
doStuff(b); // fails with good error message (DeclarationError)
let b = 1; // good
// doStuff(b); should be here, so it was a programmer typo

In general I'm in favor of disallowing as much variation as possible, so there's only one correct way to do things. In the case of uninitialized variables, most of the places we use them they are unnecessary, for example like this:

// you will stuff like this in our code base today
// because we used to have vars-on-top enabled.
// (except it will be in separate let declarations)
let a, b; // declare variables before they are used

doStuff();
doMoreStuff();
doEvenMoreStuff();

a = 1;
b = 2;
a = 3;

where it would make sense to just combine declaration and assignment:

doStuff();
doMoreStuff();
doEvenMoreStuff();

let a = 1; // declare variables where they are used
const b = 2;
a = 3;

The only place we have uninitialized variables today (and we "need" them, we cannot move the declaration to the place where they are first assigned) is state variables that are outside of singletons. It might be better to explicitly assign them as null or undefined? Or give them an initial value that makes sense? (Or move state into Backbone models or Redux)

Status quo for our code base today

If you search for "let [a-zA-Z]*;" in the project's js folder (with regex matching enabled), you'll see that there are not very many places in our code base where it's a good idea to use let a, b, c; instead of simply combining declaration and assignment. If there are only 4-5 places in the whole code where it would make sense, then I think we'd be better off just living with individual let declarations there as well (less syntax/code style rules to think about)

Another status quo problem is that ofter the let/const change, all let statements are already individual, so in order to enforce the code style, we would need to make manual changes to the code base (I don't think eslint can do it for us), but then maybe it would be a better use of our time to simply "fix" the code; combine declaration and assignment where it's possible?

And for any code we write in the future, we should try to always initialize our variables, even if they are lets. For the places where we have long changes of unitialized variables, that's probably a sign that the component has way too much internal state, which should be moved into either a Backbone model or a Redux reducer instead. Does that make sense?

[enjikaka]

Forcing initialization for variables is a not a possibility. It'll force the long ternaries mentioned above for example. And creating more functions to get around uninitialized lets is not a viable option, since more functions === less performant JS.

[eirslett]

Forcing initialization for variables is a not a possibility. It'll force the long ternaries mentioned above for example.

You're right, that's a pragmatic reason to use uninitialized let's. (Unless we use let x = undefined; before the if block:)

let x = undefined; // or just let x; if we don't enforce it.
if (stuff) {
  x = 1;
} else {
  x = 2;
}

Another option is to use a function:

function makeX() {
  if (stuff) {
    return foo;
  } else {
    return bar;
  }
}
let x = makeX();

And creating more functions to get around uninitialized lets is not a viable option, since more functions === less performant JS.

It's a very viable option, most times it's the best option. People have already written about it and discussed that topic to death, the common consensus is; if you're not in a performance-critical place (inside a render loop for example, or if you're computing digits of pi) then the overhead is insignificant. JavaScript engines are fast. DOM operations are slow, so that's where people should be doing optimization. You could probably do a million function invocations in the time it takes to modify a single DOM element (which would cause a re-layout)

[enjikaka]

let x = undefined;

Assigning to undefined just introduces redundancy. Both in the value and for the let keyword.

Legibility must always go before "being hip and write functional/immutable". Always.

It's a very viable option

Not really. Introducing functions just to be able to force use of const or an init'd let is just very bad practice and a very bad reason. Especially if the function only makes sense for that variable, and cannot be reused for anything else in the component.

[eirslett]

Clean Code God Uncle Bob on functions is a great episode that discusses back and forth where functions make sense and where they don't make sense. Too bad it costs so much money, maybe Pål can buy it for us, on the company? It's well worth the money vs. time spent/saved on code refactoring/code understanding!

[enjikaka]

What some random unknown Uncle Bob thinks doesn't matter.

Writing legible code isn't rocket science, and not harder than not making it as little complicated as it can be; by for example not making unnecessary methods just to have a variable that's immutable, having ternaries that spans multiple lines and weird variables names.

[eirslett]

Well, he is considered to be one of the influential in architecture and code layout in software engineering. He has some very good points. We could disregard his views as nonsense and be our own special snowflakes instead, if we want to. But we're obviously not getting anywhere with this discussion, at least not on the topic of one-var unitialized or not. The initialized/unitialized discussion and the extract-or-inline function stuff is just derailing. I suggest we just stick with one-var: [error, never] for now, and discuss initialized/uninitialized in another issue.

[enjikaka]

Not having

{
  "initialized": "never",
  "uninitialized": "always"
}

will give us great keyword redundancy in clusters of unintied lets. It's totally on topic discussion for this issue. :)

[eirslett]

Handy regex for matching

  let a;
  let b;

is let [a-zA-Z]*;\s*let [a-zA-Z]*;

There are 101 occurences in the code base of wimp-web-client. One can go through each of them manually, and see how many of them aren't better off being fixed (where it doesn't make sense to combine declaration and assignment).

In the degree one considers extra keywords redundancy, there is not not a "great" amount of them, only maybe 5-6. So this isn't some all-encompassing issue, "uninitialized": "always" will only affect 5-6 places in our code base. Hardly worth considering introducing an exceptional syntax for!

EDIT I can go through them all personally and fix the ones that make sense to fix, since it seems like we all agree that variables should be defined where they are used, unless that's not possible.

[eirslett]

After the patch, we're down to 27 matches of the regex in the code base. Mostly due to if/else statements (like discussed, as a valid use case for declaration without assignment) or because of state management in singletons. Even if I were in favor of "uninitialized: always" I think that's too few occurences to make a special syntax rule for. But I also tend to agree with the major part of the Internet that thinks that one-var: never is the best.

[eirslett]

Here's a final example of why uninitialized: always will be a problem:

function test () {
  let a;

  if (ok()) {
    a = 1;
  } else {
    a = 2;
  }

  let b; // declare variable where it is used

  if (ok2()) {
    b = 3;
  } else {
    b = 4;
  }

  return [a, b];
}

It's not allowed: 10:3 error Combine this with the previous 'let' statement with uninitialized variables one-var

And there are lots of long functions in our code base where I think it makes sense to have the let declarations as close to the if/else statement as possible! Is it worth the convenience of let a, b, c; in a few places that the kind of code above should be forbidden?

[enjikaka]

After a real life discussion we're going with:

"one-var": ["error", {
   "var": "always",
   "let": "never",
   "const": "never"
}]

i.e., we change let from "always" to "never". :)