Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove vars #11780

Merged
merged 5 commits into from Dec 6, 2017
Merged

Remove vars #11780

merged 5 commits into from Dec 6, 2017

Conversation

raphamorim
Copy link
Contributor

@raphamorim raphamorim commented Dec 5, 2017 •
edited

Ref: #11766 #11752 #11776
3/3 Last one @gaearon 馃憤

No more vars on packages source code :octocat:

@raphamorim raphamorim changed the title react-dom: convert packages/react-dom/src/client Remove vars Dec 5, 2017
gaearon
gaearon reviewed Dec 5, 2017
View reviewed changes
packages/react-dom/src/client/ReactDOMFiberComponent.js
@@ -887,10 +903,10 @@ export function diffHydratedProperties(
assertValidProps(tag, rawProps, getStack);

if (__DEV__) {
var extraAttributeNames: Set<string> = new Set();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this extracted to top level? Looks like a local variable to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, you right @gaearon. Updated :octocat:

@gaearon gaearon merged commit 5bd2321 into facebook:master Dec 6, 2017
@raphamorim raphamorim deleted the remove-vars branch December 6, 2017 01:48
@blling
Copy link

blling commented Dec 6, 2017 •
edited

We could lint against it but this is a bit tricky because some files have to use var. I think ideally we should have a way to override lint configuration for a subset of files. We have the same problem in Prettier config (but solved it there).

If this is the last one , should we consider lint config as a follow up?

@gaearon
Copy link
Collaborator

gaearon commented Dec 6, 2017

We could but it鈥檚 not clear to me how this should work. Ideally we want to lint ES5 and ES6 code differently depending on the directory. Happy to discuss in a separate issue if you create one.

@blling blling mentioned this pull request Dec 6, 2017
Closed
@blling
Copy link

blling commented Dec 6, 2017

@gaearon I have create one #11783

@blling
Copy link

blling commented Dec 6, 2017

I found var in some README.md docs, eg. here and here.
Are they need to replace var with const ?

@gaearon
Copy link
Collaborator

gaearon commented Dec 6, 2017

That doesn't really matter. I'm fine leaving them as is for now.

@marcelometal marcelometal mentioned this pull request Oct 25, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gaearon gaearon gaearon approved these changes

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@raphamorim @blling @gaearon @facebook-github-bot