let and const preferred over var

[muffinresearch]

Fixes mozilla/addons#5640

[muffinresearch]

An example where eslint gets it wrong. I'm modifying this later on in the file. Technically const object props can be modified but semantically using let is more correct.

[mstriemer]

I'm not sure eslint is getting it "wrong" here. const just means the value can't be reassigned, not that it can't be mutated. I think const would be needed for immutability but it doesn't enforce immutability (the type also needs to be immutable).

I think this should still be const even though it gets mutated. I'm fine with it as is though, it can get changed if someone feels like changing it.

These articles have some mildly interesting points: I did a search and found an article called ES6 const is not about immutability which goes into that and someone posted another article in the comments there about why you shouldn't use const by default, which is interesting but I'm not entirely convinced.

[tofumatt]

Coming from the Ruby world, I always feel like defining something as a constant is more about signalling intent than enforcing further assignment rules.

I like the idea of saying "we're defining X as a constant because we make no changes to it". If we're going to change the contents of a variable after we define it, we should signal that with var/let.

Being able to mess with constants isn't obvious (as evidenced by this discussion), even if it's technically correct (the best kind of correct 😉)

• tofumatt

On 5 Mar 2016, at 00:24, Mark Striemer notifications@github.com wrote:

In config/karma.config.js:

type: 'text-summary',
}];

-var newWebpackConfig = Object.assign({}, webpackConfig, {
+// eslint-disable-next-line prefer-const
+let newWebpackConfig = Object.assign({}, webpackConfig, {
I'm not sure eslint is getting it "wrong" here. const just means the value can't be reassigned, not that it can't be mutated. I think const would be needed for immutability but it doesn't enforce immutability (the type also needs to be immutable).

I think this should still be const even though it gets mutated. I'm fine with it as is though, it can get changed if someone feels like changing it.

These articles have some mildly interesting points: I did a search and found an article called ES6 const is not about immutability which goes into that and someone posted another article in the comments there about why you shouldn't use const by default, which is interesting but I'm not entirely convinced.

—
Reply to this email directly or view it on GitHub.

[mstriemer]

My concern with the signalling intent thing is that it creates a disconnect between what you think something means and what it actually allows. It's easy to say "this is going to be a constant, don't change it" but once you pass it to a function or it gets a few (dozen? hundred?) lines further down in the code all bets are off as to whether that intent will be upheld.

Realistically there's no real benefit to using const here so I'd be fine with using let and never using const unless something is actually immutable. So then if we use const it should be with something immutable like 42 or 'foo' or Immutable.Map({foo, bar}).

[mstriemer]

What I'm saying is: if we don't want this to be const then we should turn off prefer-const instead of marking some things as "ignore prefer-const".

The next value defined is const reporters but it gets pushed into in an if just after. This object could probably have the delete newWebpackConfig.entry bits removed by adding entry: undefined to this construct but I'm not sure how webpack will handle that.

[muffinresearch]

Thanks for the links and discussion points. I definitely feel like I got the wrong end of the stick re: const since I'd taken to imply 'this won't be changed' more than it it being 'this won't be rebound' despite knowing that a const object could be mutated without errors. Maybe eslint could use something like "this isn't rebound" in the error message for prefer-const.

I'll fix up this file for now - and we should all have a think about what a sane default going forward would be.

[tofumatt]

r+, cool