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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Migrating to const/let and Object Spread in more places #11535

Merged
merged 2 commits into from
Nov 19, 2017
Merged

Migrating to const/let and Object Spread in more places #11535

merged 2 commits into from
Nov 19, 2017

Conversation

raphamorim
Copy link
Contributor

@raphamorim raphamorim commented Nov 12, 2017 •
edited
Loading

@raphamorim raphamorim changed the title Migrating to const/let and Object Spread (react-dom), format folders Migrating to const/let and Object Spread (react-dom) Nov 12, 2017
@raphamorim raphamorim changed the title Migrating to const/let and Object Spread (react-dom) Migrating to const/let and Object Spread in more places Nov 12, 2017
@raphamorim raphamorim force-pushed the master branch 2 times, most recently from fe1a6e7 to 669a157 Compare November 12, 2017 23:22
clemmy
clemmy reviewed Nov 13, 2017
View reviewed changes
packages/react-dom/src/client/validateDOMNesting.js Outdated
@@ -401,7 +401,7 @@ if (__DEV__) {
return null;
};

var didWarn = {};
let didWarn = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

This can probably be a const, since it's never re-assigned.

clemmy
clemmy reviewed Nov 13, 2017
View reviewed changes
packages/react-dom/src/client/validateDOMNesting.js Outdated
var ancestorInfo = Object.assign({}, oldInfo || emptyAncestorInfo);
var info = {tag: tag, instance: instance};
const updatedAncestorInfo = function(oldInfo, tag, instance) {
let ancestorInfo = {...{}, ...(oldInfo || emptyAncestorInfo)};
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think you need ...{}

clemmy
clemmy reviewed Nov 13, 2017
View reviewed changes
packages/react-dom/src/client/validateDOMNesting.js Outdated
@@ -26,7 +26,7 @@ if (__DEV__) {
// first, causing a confusing mess.

// https://html.spec.whatwg.org/multipage/syntax.html#special
var specialTags = [
let specialTags = [
Copy link
Contributor

Choose a reason for hiding this comment

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

const?

clemmy
clemmy reviewed Nov 13, 2017
View reviewed changes
packages/react-dom/src/client/ReactDOMFiberTextarea.js
@@ -86,14 +89,13 @@ export function initWrapperState(element: Element, props: Object) {
}
}

var value = props.value;
var initialValue = value;
let initialValue = props.value;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, wonder why it was written the other way initially. 😛

Copy link
Contributor Author

@raphamorim raphamorim Nov 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

te-hehe

Do you recommend putting it back?
I think it does not make sense to keep this line. Since it is the same reference.
Tradeoff: I guess it was written for better readability.

What do you think @clemmy

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is probably fine.

clemmy
clemmy reviewed Nov 13, 2017
View reviewed changes
packages/react-dom/src/client/ReactDOMFiberTextarea.js Outdated
});
const hostProps = {
...props,
...{
Copy link
Contributor

Choose a reason for hiding this comment

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

How about:

  const hostProps = {
    ...props,
    value: undefined,
    defaultValue: undefined,
    children: '' + node._wrapperState.initialValue,
  };

clemmy
clemmy reviewed Nov 13, 2017
View reviewed changes
packages/react-dom/src/client/ReactDOMFiberOption.js Outdated
var hostProps = Object.assign({children: undefined}, props);

var content = flattenChildren(props.children);
const hostProps = {...{children: undefined}, ...props};
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar:

const hostProps = {children: undefined, ...props};

clemmy
clemmy suggested changes Nov 13, 2017
View reviewed changes
Copy link
Contributor

@clemmy clemmy left a comment

Choose a reason for hiding this comment

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

Let me know if the comments I made make sense. :)

@raphamorim
Use const/let in more places (facebook#11467)
8436555 
* Convert ReactDOMFiberTextarea to const/let
* Convert ReactDOMSelection to const/let
* Convert setTextContent to const/let
* Convert validateDOMNesting to const/let
@raphamorim
Replace Object.assign by Object Spread
d4a58d9 
* Convert ReactDOMFiberOption to Object Spread
* Convert ReactDOMFiberTextarea to Object Spread
* Convert validateDOMNesting to Object Spread
@raphamorim
Copy link
Contributor Author

raphamorim commented Nov 13, 2017 •
edited
Loading

Makes a lot of sense :D
Thanks for the review @clemmy. I squashed the changes 👍

@gaearon gaearon merged commit 3f405da into facebook:master Nov 19, 2017
Ethan-Arrowood pushed a commit to Ethan-Arrowood/react that referenced this pull request Dec 8, 2017
@raphamorim @Ethan-Arrowood
Migrating to const/let and Object Spread in more places (facebook#11535)
a0e8fc8 
* Use const/let in more places (facebook#11467)

* Convert ReactDOMFiberTextarea to const/let
* Convert ReactDOMSelection to const/let
* Convert setTextContent to const/let
* Convert validateDOMNesting to const/let

* Replace Object.assign by Object Spread

* Convert ReactDOMFiberOption to Object Spread
* Convert ReactDOMFiberTextarea to Object Spread
* Convert validateDOMNesting to Object Spread
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clemmy clemmy clemmy 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.
4 participants
@raphamorim @clemmy @gaearon @facebook-github-bot