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

Optimize things a little bit #1

Merged
merged 7 commits into from
Apr 13, 2016
Merged

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Apr 12, 2016

I updated the code to be more idiomatic Redux. I think it performs a little better in my testing.
I might be able to take another look tomorrow evening in case there’s something else I could optimize.

The basic idea here (apart from minor refactorings) is to separate independently changing data (the list of items and items themselves). This, of course, highly depends on the specific app, which is why we don’t have a proper guide for doing it, but the point is that it’s usually possible to optimize an app for specific use cases if there is a need for it.

Computing Derived Data is probably the best we have in terms of guides on how to do it, but it definitely could use some state structure refactoring advice content.

cc @ellbee for no specific reason—just in case he’s interested

@capaj
Copy link

capaj commented Apr 12, 2016

This is freaking awesome. I can't wait to see the updated charts @mweststrate!

@Kerumen
Copy link

Kerumen commented Apr 12, 2016

Just wondering why you add a shouldComponentUpdate even if the component is connected?
If I read correctly the react-redux docs, you can specifiy if a component implements a shouldComponentUpdate with the fourth argument of connect. The default is true. So by default, all components connected are pure, there is no need to write it.

No doubt you are aware of this, I didn't understand it correctly I guess.

@mweststrate
Copy link
Owner

@Kerumen you mean like in the original? For the first scenario (without selectors) TodoItem didn't use connect at all but just received it's todo item from the parent. So not using PureRenderMixin would have caused all TodoItem components to re-render if a todo changes, instead of only the affected ones.

@Kerumen
Copy link

Kerumen commented Apr 12, 2016

No I was asking for the @gaearon rewrite. For instance here.

@michaseel
Copy link

@gaearon Could you update the unit tests too? I would like to see how you test stateless functional components!

@gaearon
Copy link
Contributor Author

gaearon commented Apr 12, 2016

Well, it’s not that much of an improvement but it’s definitely better. Still can’t avoid going through 10K subscribers. There may be small things to optimize in React Redux as well so I’ll look there.

I removed shouldComponentUpdate later, please see the complete change list. I added it for testing in one of the commits but decided it is unnecessary.

No, alas, I won’t be adding tests here, you should check out Redux repo (and open PRs to it) to see how to test components.

@slorber
Copy link

slorber commented Apr 12, 2016

Still can’t avoid going through 10K subscribers.

I guess 10k subscribers is better than 10k items list reconciliation.

But there are potential ways to avoid calling those 10k subscribers on every item change. Didn't have time to work on something concrete yet but it can be optimized further imho. See reduxjs/react-redux#269

@gaearon
Copy link
Contributor Author

gaearon commented Apr 12, 2016

Yeah, there are definitely ways to solve this even with vanilla Redux. The goal here is to avoid complicating the code beyond recognition 🙂

import React, { PropTypes } from 'react'
import Header from '../components/Header'
import MainSection from '../components/MainSection'
import * as TodoActions from '../actions'
Copy link

Choose a reason for hiding this comment

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

This TodoActions import is useless here, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, missed this one. Doesn't affect the perf though.

@mweststrate
Copy link
Owner

Thanks for the contributions! Will re-run benchmarks tomorrow.

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.

7 participants