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

Confusing documentation #83

Closed
arkwright opened this issue Sep 1, 2015 · 3 comments · Fixed by #84
Closed

Confusing documentation #83

arkwright opened this issue Sep 1, 2015 · 3 comments · Fixed by #84

Comments

@arkwright
Copy link

Redux docs state:

[We] wrap the components we want to connect to Redux with the connect() function from react-redux. Try to only do this for a top-level component, or route handlers. While technically you can connect() any component in your app to Redux store, avoid doing this too deeply, because it will make the data flow harder to trace.

https://rackt.github.io/redux/docs/basics/UsageWithReact.html

But, in a seeming contradiction, react-redux README states:

Don’t do this! It kills any performance optimisations because TodoApp will rerender after every action.
It’s better to have more granular connect() on several components in your view hierarchy.

I'm not sure whose advice to follow!

@mindjuice
Copy link
Contributor

Try to only do this for a top-level component

Notice it says "a" top-level component, not "the" top-level component. You can, and probably should, connect() multiple times for top-level components (e.g., header, sidebar, main content, footer). You could make the argument that choosing the route handler would be "the" top-level though.

In my opinion, more calls to connect() doesn't actually make the data harder to trace, but easier. I can see immediately where the data is coming from in a connect()ed component rather than tracing it up through props.

The downside though, is that it ties a component to a given app, because the component needs to know the shape of the state object. If you don't want to reuse the code though, then that's not really a downside.

It's generally better to have a handful of "smart" components that know about the app, and have them rely on "dumb" components that render entirely from props.

If you're not using it already, look into reselect too. They are about to go 1.0.

You may also be able to take advantage of React's PureRenderMixin to restrict how deep the re-rendering goes.

@gnoff
Copy link
Contributor

gnoff commented Sep 2, 2015

@arkwright I believe you are slightly misinterpreting the point of the the readme where is says

Don’t do this! It kills any performance optimisations because TodoApp will rerender after every action.

The example is showing an connect select function that maps the entire state to the connected component

export default connect(state => state)(TodoApp);

In any application of reasonable size this is likely unnecessary since parts of your state are likely not of concern for the given connected component. The point about performance is recognizing that in this given example the component will be re-rendered anytime any state changes even if it isn't relevant to the connected component.

I think the readme example could probably make this a little more clear so that future readers don't also come away with the impression you did.

Hopefully that helps clear things up.

Thanks,
Josh

gnoff added a commit that referenced this issue Sep 2, 2015
clarifying admonition against listening to the entire state in `connect()`

resolves #83
@gnoff gnoff closed this as completed in #84 Sep 2, 2015
@gaearon
Copy link
Contributor

gaearon commented Sep 2, 2015

👍

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 a pull request may close this issue.

4 participants