-
Notifications
You must be signed in to change notification settings - Fork 397
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
let and const preferred over var #71
Merged
muffinresearch
merged 1 commit into
mozilla:master
from
muffinresearch:favour-let-and-const-over-var
Mar 4, 2016
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thinkconst
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 useconst
by default, which is interesting but I'm not entirely convinced.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😉)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 usinglet
and never usingconst
unless something is actually immutable. So then if we useconst
it should be with something immutable like42
or'foo'
orImmutable.Map({foo, bar})
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I'm saying is: if we don't want this to be
const
then we should turn offprefer-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 thedelete newWebpackConfig.entry
bits removed by addingentry: undefined
to this construct but I'm not sure how webpack will handle that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.