-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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
React: bump to 0.5.1, fix bugs, improve perf, improve style #702
Conversation
- 2x: The unminified version contains invariants that shouldn't be used in production and slow it down. - 3x: Adding a shouldComponentUpdate override in order to short-cut re-rendering the component when props and state didn't change. In order to do that, we need to avoid doing mutations by doing a shallow copy and only modifying what changed - When React 0.5.0 will be released, it will give another 1.5x. Other miscellaneous changes: - Update class="..." to className="..." to work in 0.5.0 (still works in 0.4.1) - Use prop="..." instead of prop='...' - Use filter(f, this) instead of filter(f.bind(this)) to prevent a function allocaton
This looks good to me, thanks for taking it over |
Looks pretty good to me. |
@@ -12,7 +12,7 @@ | |||
<div id="benchmark"></div> | |||
|
|||
<script src="bower_components/todomvc-common/base.js"></script> | |||
<script src="bower_components/react/react.js"></script> | |||
<script src="bower_components/react/react.min.js"></script> |
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.
we prefer full source so the demo apps are easily debuggable.
I noticed that the active filter (All - Active - Completed) isn't getting the |
Weird, I tested this and didn't notice the bug with the all/active/completed; must have been cached. Seems to work now. |
This is based on #693 and supersedes it. It also bumps the React version and fixes the issues discussed in the review. We should be good to go with this.