-
Notifications
You must be signed in to change notification settings - Fork 47.6k
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 the homepage with ES6 #7868
Conversation
return ( | ||
<div>Seconds Elapsed: {this.state.secondsElapsed}</div> | ||
); | ||
} | ||
}); | ||
} | ||
|
||
ReactDOM.render(<Timer />, mountNode); |
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.
Do you think it would be more similar to real usage if you used document.getElementById('root') or similar here, rather than mountNode?
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.
Yes, but this would require some further changes to how this page is set up (e.g. passing IDs as props to the code editor). Can do separately.
id: Date.now() | ||
}; | ||
this.setState({ | ||
items: [...this.state.items, newItem], |
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.
how about just using concat to avoid ...
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.
Also - wasn't someone talking recently about, you shouldn't use this.state.foo in setState, because there's some race condition if you do it twice per update cycle? I really prefer the look of this code, but, I just want to make sure we're not suggesting something buggy.
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.
It used to be concat
but it looked pretty crowdy and I thought this could save some space. IMO it's not hard to guess what it does even if you don't use ES6. But happy to change back to concat if you think that was better. (Many people don't know concat()
exists or what it does either.) It's also not really the point of the example, and the code is more for illustrative purposes rather than for direct learning.
Also - wasn't someone talking recently about, you shouldn't use this.state.foo in setState
Ideally you shouldn't but the alternative is too much syntax noise for this example so I think it's okay.
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.
It's not quite a race condition, it's that setState
is potential asynchronous and/or batched. I don't think it's a good idea for the official docs to implicitly communicate that this is okay. Nobody will know this is a problem unless we're explicit about it.
tick() { | ||
this.setState({ | ||
secondsElapsed: this.state.secondsElapsed + 1 | ||
}); |
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.
Did we reach a decision on whether to use the updater form here or not? Even though it doesn't matter for a Timer where state is only updating once a second, people will see this and use it elsewhere.
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.
Fixed.
id: Date.now() | ||
}; | ||
this.setState({ | ||
items: [...this.state.items, newItem], |
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.
It's not quite a race condition, it's that setState
is potential asynchronous and/or batched. I don't think it's a good idea for the official docs to implicitly communicate that this is okay. Nobody will know this is a problem unless we're explicit about it.
THANK YOU !
@acdlite commented on this pull request.In docs/_js/examples/timer.js:> - this.setState({secondsElapsed: this.state.secondsElapsed + 1});
);
|
LGTM |
* Update the homepage with ES6 * Avoid array spread and stale state
THANK YOU !
@gaearon commented on this pull request.In docs/_js/examples/timer.js:> - this.setState({secondsElapsed: this.state.secondsElapsed + 1});
|
YOU ARE WELCOME !
LGTM— |
YOU ARE WELCOME ! On Tuesday, October 4, 2016 2:34 PM, Kevin Lacker [email protected] wrote: LGTM— — |
* Update the homepage with ES6 * Avoid array spread and stale state
Sending this against master because it's independent and self-contained.
A few changes to unpack here:
babel-standalone
because it is much newer. This is already done in [docs] Use react-standalone #7668 but since that didn't get merged yet I'm just doing it here, and we can clean up other places as part of New Docs #7864. There were no other places where this particular hosted file was used.render()
to be at the top of Todos example so that you see it immediately and it matches what you see on the screen.Here's how it looks now:
Compiled JS view: