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.

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 - Improve performance by 6x #693

Closed
wants to merge 1 commit into from
Closed

Conversation

vjeux
Copy link
Contributor

@vjeux vjeux commented Oct 6, 2013

  • 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

 - 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
todo.completed = !todo.completed;
this.setState({todos: this.state.todos});
var newTodos = this.state.todos.map(function (t) {
if (t !== todo) {
Copy link
Member

Choose a reason for hiding this comment

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

don't use one char variable names

@sindresorhus
Copy link
Member

@petehunt @chenglou can you review?

@@ -92,8 +96,14 @@
},

save: function (todo, text) {
todo.title = text;
this.setState({todos: this.state.todos, editing: null});
var newTodos = this.state.todos.map(function (t) {

Choose a reason for hiding this comment

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

I think this could be a bit cleaner: Thoughts?

var newTodos = this.state.todos.map(function(t) {
  return t !== todo ? t : Utils.extend({}, t, {title: text});
});

Copy link
Member

Choose a reason for hiding this comment

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

I prefer the shorter one. @sindresorhus?

Copy link
Member

Choose a reason for hiding this comment

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

ditto

@petehunt
Copy link
Contributor

petehunt commented Oct 6, 2013

@sindresorhus @chenglou and I gave this a review last night before submitting. Outside of the single char variable name we're good with this. @jordwalke 's comments seem like a good idea though.

@petehunt
Copy link
Contributor

Can we close this in favor of #702?

@vjeux vjeux closed this Oct 29, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants