Replace inappropriate uses of let with const

[pixelzoom]

(1) Replace inappropriate uses of let with const. Use let only for things that will be modified.

(2) Investigate whether we need another lint rule.

Slack discussion

Chris Malley [11:53 AM]
Hey devs, I’m looking through recent common code changes, and I suspect that some devs are confused about when to use const vs let. Use const for things that will not change (constants), use let for things that need to change. Looking only in scenery-phet, I see improper uses of let in: Utterance, BorderAlertsDescriber, HeaterCooler*, DirectionEnum, MovementDescriber, A11yGrabDragNode. (edited)

Example in BorderAlertsDescriber:

// a11y strings
let leftBorderAlertString = SceneryPhetA11yStrings.leftBorderAlert.value;
let rightBorderAlertString = SceneryPhetA11yStrings.rightBorderAlert.value;
let topBorderAlertString = SceneryPhetA11yStrings.topBorderAlert.value;
let bottomBorderAlertString = SceneryPhetA11yStrings.bottomBorderAlert.value;

These are all constants, should be const.
This is the whole point of moving away from var, and using const and let instead.

Michael Kauzmann [11:58 AM]
Hey all those files are mine. Will you make me an issue and I'll take a look.

Chris Malley [11:58 AM]
Not all yours, but I will make an issue.

Denzell Barnett [11:58 AM]
Assign me as well for HeaterCooler*

Chris Malley [11:59 AM]
I’m going to put the issue in tasks repo, since this looks like it cuts across all repos.

Sam Reid [12:00 PM]
Can a lint rule catch lets that should be const?

[samreid]

Lint can catch this:

    "prefer-const": ["error", {
      "destructuring": "any",
      "ignoreReadBeforeAssign": false
    }],

And lint can auto-fix this. There are currently 564 occurrences of this across our codebase.

[zepumph]

I'm going to go through and try to hit a bunch of these that are my fault.

[zepumph]

As I'm going through this I'm finding that there is one issue with the lint rule above. There is one case that requires either ignoring the lint rule, or refactoring.

let myConst;

// do some stuff 
let otherVar = 2;
myConst = 2 + otherVar;

// I never reassign

This will be triggered by the prefer-const rule because the value is not reassigned. Unfortunately the following is illegal:

const hello;
hello = 'I say hello now';

So either we need to disable the rule, or code around it.

I'll be committing shortly, so far we are down around 300 lint errors.

[pixelzoom]

Declaring a variable without initializing its value is a code smell. Both of the examples in the preceding comment have that smell. In both of the examples, simply wait to declare until you assign.

[zepumph]

Scratch that I'm going to do all of them.

[zepumph]

Declaring a variable without initializing its value is a code smell. Both of the examples in the preceding comment (for let and const) have that smell. In both of the examples, simply wait to declare until you assign.

I agree except that sometimes it is nice to declare in a larger scope, and then initialized in a loop or something.

[pixelzoom]

Then use let something = initialValue in the outer scope, and set the value of something in the inner scope.

Example from GQCheckBox:

      let content = null;
      if ( options.icon ) {
        content = new HBox( {
          align: 'center',
          spacing: 8,
          children: [ textNode, options.icon ]
        } );
      }
      else {
        content = textNode;
      }

If you really want to be bulletproof, assert( content, '...' ) after the else statement.

[zepumph]

Sounds good to me, thanks.

[zepumph]

I committed all of the fixes for this, and also the lint rule. We can tweak the rule if we want to, but I figured that it was best to just get it out there, since there were a lot of places it was wrong. Marking for dev meeting as a PSA that this is a rule, and to discuss if there are questions or concerns with it.

[zepumph]

And lint can auto-fix this

I'm not sure we need this. I think it is good for devs to manually have to write the code in the way we decide is best.

[samreid]

And lint can auto-fix this

I agree we should not have auto-fix on by default. If anything, autofix should be considered for initial batches, not ongoing development.

[Denz1994]

Lint rule has been created by @zepumph. Devs have been made aware.