-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
Update variables to ES6 #2169
Update variables to ES6 #2169
Conversation
I don't see any problem with either. Should we also add the eslint rules to forbid |
Sure, might as well. |
I remember seeing a chrome benchmark of |
@@ -115,13 +115,13 @@ export default function combineReducers(reducers) { | |||
finalReducers[key] = reducers[key] | |||
} | |||
} | |||
var finalReducerKeys = Object.keys(finalReducers) | |||
const finalReducerKeys = Object.keys(finalReducers) | |||
|
|||
if (NODE_ENV !== 'production') { | |||
var unexpectedKeyCache = {} |
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.
This is one spot, line 121, where I couldn't change to let, without errors failing.
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.
A fix I had was declaring let unexpectedKeyCache
above the conditional check. But I don't know if you guys want an undefined variable floating around in production.
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.
On second thought, the let
fix should be fine, because if it's using variable hoisting as it is. Then defining let before the block is no problem. Let me know what you guys think.
We might have to do another change to add the eslint rule for forbidding |
You could disable the rule on that line with |
@jimbolla I think defining |
Everything is good to go now. I can also add the eslint rules too. Just let me know what level you want them at. |
Error is fine I guess. |
Eslint rules added. All lint rules passed. All tests passed. Should be good to go now. |
LGTM! |
* Update variables to ES6 * Fix var for sanityError * Fix var unexpectedKeyCache * Add eslint rules
Fixes #2168. Updated all variable declarations to respective ES6 variants when applicable. All tests passed. No other code changed.