-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
Recommended usage of connect() #419
Comments
I strongly agree with Brent. This was much of the fundamental thinking behind something like Relay and leads to higher cohesion. |
Maybe we should just say “people have difference preferences here”. |
@gaearon - perhaps a section on the tradeoffs which includes a demonstration of how it complicates tracing data flow would be useful. I could pitch in on my side of the preferences for that. |
Cool. Let's keep it open and revisit after the initial docs settle down. |
Sounds good @gaearon! :) Ping me when you'd like to revisit |
I'm pretty new to Redux, but this has been a pretty big pain point for me during the last year, building an app where we've changed the data structure quite a lot. So have to say I really agree with @brentvatne on this one. Are there other problems than tracing data flow when connect()-ing a lot of components? Would there be a performance penalty for example? |
No, it's exactly the opposite: you get better performance by giving up top-down flow. |
Because you can avoid unnecessary re-renders of middle-level components? |
Yes. |
To add to the discussion: I'm mostly limiting my usage of connect to route components. When I have a page that could use an extra smart component (e.g. a form modal), my workaround has been to pass the element or node, and have the dumb component render it. It means you have a bit more boilerplate, but testing the dumb component is still easy. I'm still experimenting with this, but I think this might be the best way to go about composing smart components without giving up easy testability. To give a rough example: Foo.jsx export class Foo extends Component {
render () {
return (
<div className='foo'>
{/* foo stuff going on in here */}
{this.props.Bar}
</div>
)
}
} FooContainer.jsx @connect(getState, getActions)
export class FooContainer extends Component {
render () {
return (
<Foo
Bar={<BarContainer/>}
{...this.props}
/>
)
}
} |
@brentvatne If you feel like writing something, figure out where to fit it into the current doc structure, and feel free to go ahead! :-) |
2015/10/19 update: I've improved upon this comment and released it as Continued from #475, I'll describe what I've come up with, although this discussion probably now belongs in Modular Providers Using Sideways AssignmentLong story short, the approach I've taken revolves around modular providers that you can assign to any component. This allows for truly "dumb" components, enforces a maximum separation of concerns, and makes it possible to very easily and quickly use and share any number of interchangeable providers. It also enforces a more efficient way to update components. So... I got rid of the Example "Dumb" Component// components/Branch.js
import React, { Component } from 'react';
import provide from '../utilities/provide.js';
import { branchName, tree, toggle, open, theme } from '../common/propTypes.js';
import Limbs from './Limbs.js';
@provide // maybe require specifying expected props? e.g., @provide('theme')
export default class Branch extends Component {
static propTypes = { branchName, tree, toggle, open, theme };
onClick(event) {
const { branchName, toggle } = this.props; // toggle is from a provider
event.stopPropagation();
toggle(branchName);
}
render() {
const props = this.props;
const { branchName, tree, open, theme } = props; // latter 3 from providers
const classes = theme.sheet.classes || {};
const imgSrc = open ? 'folder-open.png' : 'folder-closed.png';
return (
<div
onClick={::this.onClick}
className={classes.branch}
>
<h4 className={classes.branchName}>
<img
className={classes.branchIcon}
src={theme.imagesDir+imgSrc}
/>
<span>{branchName}</span>
</h4>
<Limbs
tree={tree}
open={open}
/>
</div>
);
}
} Example Provider// providers/toggle.js
import createProvider from '../utilities/createProvider.js';
export const TOGGLE = 'TOGGLE';
export const actions = {
toggle(fullPath) {
return { type: TOGGLE, fullPath };
}
};
export const reducers = {
open(state = {}, action) {
switch (action.type) {
case TOGGLE:
const { fullPath } = action;
return { ...state, [fullPath]: !state[fullPath] };
default:
return state;
}
}
};
function merge (stateProps, dispatchProps, parentProps) {
return Object.assign({}, parentProps, {
open: !!stateProps.open[parentProps.fullPath]
});
}
export const provider = createProvider(actions, reducers, merge);
export default provider; Sideways AssignmentAs mentioned, the idea is to be able to easily assign arbitrary providers to "dumb" components. So when mounting your app, you can do that like this: import React from 'react';
import { Provider } from 'react-redux';
import assignProviders from './utilities/assignProviders.js';
import createStoreFromProviders from './utilities/createStoreFromProviders.js';
import dark from './themes/dark.js';
import github from './sources/github.js';
import * as providers from './providers/index.js';
import * as components from './components/index.js';
const { packageList, sources, toggle, theme } = providers;
const { Branches, Branch, Limbs, Limb } = components;
const initialState = {
packageList: [
'github:gaearon/react-redux@master',
'github:loggur/branches@master',
'github:rackt/redux@master'
],
sources: {
github: github({
token: 'abc123',
auth: 'oauth'
})
},
open: {
'github': true,
'github:gaearon': true,
'github:gaearon/react-redux@master': true,
'github:rackt': true,
'github:rackt/redux@master': true
},
theme: dark
};
const store = createStoreFromProviders(providers, initialState);
assignProviders({ theme }, components);
assignProviders({ packageList }, { Branches });
assignProviders({ sources }, { Branches, Branch });
assignProviders({ toggle }, { Branch, Limb });
React.render(
<Provider store={store}>
{() => <Branches/>}
</Provider>,
document.getElementById('root')
); Hopefully this is all pretty straightforward, but I'd be glad to go into more detail and add comments for clarification if necessary. Additional CodeYou'll probably notice a handful of utility functions being imported in these examples. These are tiny modules (3 of the 4 are only a few lines long) which are basically just a combination of existing // utilities/createProvider.js
import { bindActionCreators } from 'redux';
import { connect } from 'react-redux';
/**
* Creates an object to be used as a provider from a set of actions and
* reducers.
*
* @param {Object} actions
* @param {Object} reducers
* @param {Function} merge Optional
* @return {Object}
* @api public
*/
export default function createProvider (actions, reducers, merge) {
return {
mapState(state) {
const props = {};
for (let key in reducers) {
props[key] = state[key];
}
return props;
},
mapDispatch(dispatch) {
return bindActionCreators(actions, dispatch);
},
merge
};
} // utilities/createStoreFromProviders.js
import { createStore, combineReducers } from 'redux';
import { createStore, applyMiddleware, combineReducers } from 'redux';
/**
* Creates a store from a set of providers.
*
* @param {Object} providers
* @param {Object} initialState Optional
* @return {Object}
* @api public
*/
export default function createStoreFromProviders (providers, initialState) {
const reducers = {};
const middleware = [];
let create = createStore;
for (let key in providers) {
let provider = providers[key];
Object.assign(reducers, provider.reducers);
if (provider.middleware) {
if (Array.isArray(provider.middleware)) {
for (let mid of provider.middleware) {
if (middleware.indexOf(mid) < 0) {
middleware.push(mid);
}
}
} else if (middleware.indexOf(provider.middleware) < 0) {
middleware.push(provider.middleware);
}
}
}
if (middleware.length) {
create = applyMiddleware.apply(null, middleware)(createStore);
}
return create(combineReducers(reducers), initialState);
} // utilities/assignProviders.js
/**
* Assigns each provider to each component. Expects each component to be
* decorated with `@provide` such that it has an `addProvider` static method.
*
* @param {Object} providers
* @param {Object} components
* @api public
*/
export default function assignProviders (providers, components) {
for (let providerName in providers) {
let provider = providers[providerName];
if (provider.default) {
provider = provider.default;
} else if (provider.provider) {
provider = provider.provider;
}
for (let componentName in components) {
let addProvider = components[componentName].addProvider;
if (typeof addProvider === 'function') {
addProvider(providerName, provider);
}
}
}
} And last but not least, we have the // utilities/provide.js
import React, { Component, PropTypes } from 'react';
import createStoreShape from 'react-redux/lib/utils/createStoreShape';
import shallowEqual from 'react-redux/lib/utils/shallowEqual';
import isPlainObject from 'react-redux/lib/utils/isPlainObject';
import wrapActionCreators from 'react-redux/lib/utils/wrapActionCreators';
import invariant from 'invariant';
const storeShape = createStoreShape(PropTypes);
const defaultMapState = () => ({});
const defaultMapDispatch = dispatch => ({ dispatch });
const defaultMerge = (stateProps, dispatchProps, parentProps) => ({
...parentProps,
...stateProps,
...dispatchProps
});
// Helps track hot reloading.
let nextVersion = 0;
export default function provide (WrappedComponent) {
const version = nextVersion++;
const providers = [];
let shouldSubscribe = false;
function getDisplayName () {
return ''
+'Provide'
+(WrappedComponent.displayName || WrappedComponent.name || 'Component')
+'('+providers.map(provider => provider.name).join(',')+')';
}
function addProvider (name, { mapState, mapDispatch, merge }) {
if (Boolean(mapState)) {
shouldSubscribe = true;
}
providers.push({
name,
mapState: mapState || defaultMapState,
mapDispatch: isPlainObject(mapDispatch)
? wrapActionCreators(mapDispatch)
: mapDispatch || defaultMapDispatch,
merge: merge || defaultMerge
});
Provide.displayName = getDisplayName();
}
function computeStateProps (store) {
const state = store.getState();
const stateProps = {};
for (let provider of providers) {
let providerStateProps = provider.mapState(state);
invariant(
isPlainObject(providerStateProps),
'`mapState` must return an object. Instead received %s.',
providerStateProps
);
Object.assign(stateProps, providerStateProps);
}
return stateProps;
}
function computeDispatchProps (store) {
const { dispatch } = store;
const dispatchProps = {};
for (let provider of providers) {
let providerDispatchProps = provider.mapDispatch(dispatch);
invariant(
isPlainObject(providerDispatchProps),
'`mapDispatch` must return an object. Instead received %s.',
providerDispatchProps
);
Object.assign(dispatchProps, providerDispatchProps);
}
return dispatchProps;
}
function computeNextState (stateProps, dispatchProps, parentProps) {
const mergedProps = {};
for (let provider of providers) {
let providerMergedProps = provider.merge(
stateProps, dispatchProps, parentProps
);
invariant(
isPlainObject(providerMergedProps),
'`merge` must return an object. Instead received %s.',
providerMergedProps
);
Object.assign(mergedProps, providerMergedProps);
}
return mergedProps;
}
const Provide = class extends Component {
static displayName = getDisplayName();
static contextTypes = { store: storeShape };
static propTypes = { store: storeShape };
static WrappedComponent = WrappedComponent;
static addProvider = addProvider;
shouldComponentUpdate(nextProps, nextState) {
return !shallowEqual(this.state.props, nextState.props);
}
constructor(props, context) {
super(props, context);
this.version = version;
this.store = props.store || context.store;
invariant(this.store,
`Could not find "store" in either the context or ` +
`props of "${this.constructor.displayName}". ` +
`Either wrap the root component in a <Provider>, ` +
`or explicitly pass "store" as a prop to "${this.constructor.displayName}".`
);
this.stateProps = computeStateProps(this.store);
this.dispatchProps = computeDispatchProps(this.store);
this.state = { props: this.computeNextState() };
}
recomputeStateProps() {
const nextStateProps = computeStateProps(this.store);
if (shallowEqual(nextStateProps, this.stateProps)) {
return false;
}
this.stateProps = nextStateProps;
return true;
}
recomputeDispatchProps() {
const nextDispatchProps = computeDispatchProps(this.store);
if (shallowEqual(nextDispatchProps, this.dispatchProps)) {
return false;
}
this.dispatchProps = nextDispatchProps;
return true;
}
computeNextState(props = this.props) {
return computeNextState(
this.stateProps,
this.dispatchProps,
props
);
}
recomputeState(props = this.props) {
const nextState = this.computeNextState(props);
if (!shallowEqual(nextState, this.state.props)) {
this.setState({ props: nextState });
}
}
isSubscribed() {
return typeof this.unsubscribe === 'function';
}
trySubscribe() {
if (shouldSubscribe && !this.unsubscribe) {
this.unsubscribe = this.store.subscribe(::this.handleChange);
this.handleChange();
}
}
tryUnsubscribe() {
if (this.unsubscribe) {
this.unsubscribe();
this.unsubscribe = null;
}
}
componentDidMount() {
this.trySubscribe();
}
componentWillReceiveProps(nextProps) {
if (!shallowEqual(nextProps, this.props)) {
this.recomputeState(nextProps);
}
}
componentWillUnmount() {
this.tryUnsubscribe();
}
handleChange() {
if (this.recomputeStateProps()) {
this.recomputeState();
}
}
getWrappedInstance() {
return this.refs.wrappedInstance;
}
render() {
return (
<WrappedComponent ref='wrappedInstance' {...this.state.props} />
);
}
}
if ((
// Node-like CommonJS environments (Browserify, Webpack)
typeof process !== 'undefined' &&
typeof process.env !== 'undefined' &&
process.env.NODE_ENV !== 'production'
) ||
// React Native
typeof __DEV__ !== 'undefined' &&
__DEV__ //eslint-disable-line no-undef
) {
Provide.prototype.componentWillUpdate = function componentWillUpdate () {
if (this.version === version) {
return;
}
// We are hot reloading!
this.version = version;
// Update the state and bindings.
this.trySubscribe();
this.recomputeStateProps();
this.recomputeDispatchProps();
this.recomputeState();
};
}
return Provide;
} One thing worth mentioning about the Instead of this (which you'd see with the original LimitationsI only began learning about I came up with this particular approach because I really like the idea of having truly "dumb" components that can have any provider assigned to them. It enforces a true separation of concerns, and it allows for any number of easily interchangeable providers as modules. Furthermore, if @gaearon likes this approach and thinks it falls within the scope of |
The direction we've taken now is that we are going to use Immutable.js for our state (both App State and UI State; we keep some UI State in the components), and then use the PureRenderMixin for all our components. Edit: I just realized that it won't eliminate that performance penalty for middle-level components if one of their lower level components still need to re-render, but it should still eliminate a lot of the overhead. Any experience with that? |
@danmaz74 We've been hitting perf issues in scenarios like that (using not redux, but a very similar home made lib). We have a pretty complex app though, with some very "expensive" components. Also, as an app grows bigger, injecting data in more places than just the top level can help you avoid creating implicit dependencies between components and avoid having parents know too much about the data requirements of their children. |
@eldh thanks for the update, that makes sense. We'll keep it in mind while going on :) |
Any updates on this? timbur's example makes perfect sense to me while the single connect practice I don't quite understand. I would be interested in the counter argument, i.e. “people have difference preferences here”. Using a single connect one would need a huge mapStateToProps function to transform the state all the way to e.g. a 10 component deep hierarchy... I'm a bit confused what the thought behind that is or whether I am misunderstanding something... |
Nobody advocates a single connect.
The "single" only refers to small apps like the one we create in the example. Please feel free to amend the docs to better clarify this. I am now busy with other projects so please don't expect this issue to get any movement unless somebody makes a PR. You can do it too. |
Thank you for the clarification. |
Finally got around to releasing |
That looks very good, thanks! |
I've just started a new redux project, and am using connect for 'smart' components - this makes far more sense to me, and if there's a perf benefit there's an extra win. Conversely, if you bubbled all control up to the main app or routers, there's a complete loss of SRP - how big does your app have to get before you start breaking things down? I'm even thinking about organising related elements into component folder - ie. put a reducer in beside it's main component, etc. Ultimately, I believe that redux/flux are a huge benefit for predictable state, but such a mental shift from standard mv-whatever that has made UI app development simple & accessible to anyone, that eventually flux will be abstracted away and we'll move back to something that looks more like mv*. |
This is being fixed in #1285. |
We no longer discourage creating container components in the updated docs. |
Hey I wrote about some stuff that might help here. :) https://medium.com/@timbur/react-automatic-redux-providers-and-replicators-c4e35a39f1 |
I think #419 (comment) can help with #1353, too. |
@timbur Awesome article! Could you please also share your thoughts about this question: reduxjs/react-redux#278 |
I'm not sure whether this is blindingly obvious but I feel it's worth adding it here for clarity, as I think a lot of what's been said is perhaps a bit abstract for someone new to redux coming to this thread. When I first started using redux I misguidedly dotted "containers" (connected components) inside (just plain old dumb) "components" because I thought my app wasn't going to require much decoupling. How wrong I was. When I realised I needed to reuse many of these components I had to do a fair bit of refactoring and moved a lot of smart stuff right to the top of the tree but this soon became unwieldy; a "provider" at the top providing the context (as it should) but also basically ALL of the app (which it shouldn't). The way I've found best to approach it is to have a hierarchy of containers that compose dumb components, with a provider at the top. Containers should only live inside other containers. Your app should be a hierarchy of containers that use components to present their data. Often a good way to do this with lists is to pass IDs down through smart components. The need for an ID is a sign that something belongs to the app's domain. So where possible take lists of IDs in one container and pass them down to another container which can use the IDs to get the information it wants. Within each container use a component to render that information without the need of the ID. Below I've mocked out a (convoluted) example of how to pass down the connected parts of the app through a container hierarchy that uses components to display them. // Provider component that renders some containers and some components and provides the store
class TodoAppProvider {
constructor() {
// setup store etc.
}
render() {
return (
<Provider store={this.store}> {/* Provider from 'react-redux' */}
<AppLayoutComponent title="My Todos" footer={<TodoFooter />}>
<TodoListsContainer />
</AppLayoutComponent>
</Provider>
);
}
);
// AppLayoutComponent
// Lots of nice css, other dumb components etc. no containers!
export default const AppLayoutComponent = ({ title, children, footer }) => (
<header>
{title}
</header>
<main>
{children /* This variable can be a container or components but it's not hardcoded! */}
</main>
<footer>
{footer}
</footer>
);
// TodoFooter
// Another dumb component
export default const TodoFooter = () => (
<footer>
© {Date.now() /* we are copyrighted to the millisecond */}
</footer>
);
// TodoListsContainer
// Smart component that renders all the lists
class TodoListsContainer extends React.Component {
render() {
return () {
<div>
{todoLists.map(id => (
{/* this container renders another container */ }
<TodoListContainer key={id} todoListId={id} />
))}
</div>
}
}
}
const mapStateToProps = state => ({
todoLists: getTodoLists(state),
});
export default connect(mapStateToProps)(TodoListsContainer);
// TodoListContainer
// Gets the props and visibleTodo IDs for the list
class TodoListContainer {
render() {
const { id, title, visibleTodos } = this.props;
return (
<div>
{/* Render a component but passes any connected data in as props / children */}
<TodoListPanelComponent title={title}>
{visibleTodos.map(id => (
<TodoContainer todoId={id} />
))}
</TodoListPanelComponent>
</div>
);
}
}
const mapStateToProps = (state, { todoListId }) => ({
...getTodoList(state, todoListId), // A todo object (assume we need all the attributes)
visibleTodos: getVisibleTodos(state, todoListId), // returns ids
});
export default connect(mapStateToProps)(TodoListContainer);
// TodoListPanelComponent
// render the panel to sit the todos in
// children should be todos
// No containers!
export default const TodoListPanelComponent = ({ title, children }) => (
<div>
<h3>{title}</h3>
<div>
{children}
</div>
</div>
);
// TodoContainer
// This just wraps the TodoComponent and passed the props
// No separate class or JSX required!
const mapStateToProps = (state, { todoId }) => ({
...getTodo(state, todoId),
});
const mapDispatchToProps = (dispatch, { todoListId }) => ({
handleFilter: () => dispatch(hideTodo(id)), // Pass ALL smart stuff in
});
export default connect(mapStateToProps, mapDispatchToProps)(TodoComponent); // Passing in the component to connect
// TodoComponent
// Render the component nicely; again, as all of its connected stuff passed in
// The FilterLinkContainer is an example of a smell that will come back to bite you!
export default const TodoComponent = ({ content, isComplete, handleFilter }) => (
<div>
<div>
{content}
</div>
<div>
{isComplete ? '✓' : '✗'}
</div>
<div>
{/* Don't do this, can't re-use TodoComponent outside the app context! */}
<FilterLinkContainer />
{/* Instead do this (or similar), component can be reused! */}
<Link onClick={handleFilter}>
'Filter'
</Link>
</div>
</div>
); So here the container hierarchy is Ultimately, the way I like to think of it is as if you were to initially create a tree of connected components that map your data into a useful way that your UI will care about. However there is no UI whatsoever, just a tree of state mapped through a hierarchy of just containers. After that, you go through and sprinkle presentational components inside those containers to actually display the data in whatever way you want (i.e. in the DOM). Obviously it's not useful to write your app in this two-pass way but I found it useful to conceptualise the model like this. |
Is there a performance penalty (or another architectural reason) for using
I'm being lazy here and didn't combine the two versions of |
@timotgl : Dan is no longer an active maintainer - please don't ping him directly. I was about to say that your question is answered in the Redux FAQ entry on connecting multiple components, but it looks like you're asking about something different - deliberately wrapping multiple connections around a single component? I can't say I've ever seen anyone do that before, and I've seen a lot of Redux code. Personally, I'd suggest trying a different approach. Either have a "common props" HOC or something that mostly just passes those through to its children, or use selectors to retrieve the common props in the specific component's |
@markerikson Sorry didn't know that, at-mention removed. So first of all, it works, and the component appears like any other connected component in the react dev tools, it doesn't have an additional wrapper or anything like that. I decided against a HOC because I didn't want to involve the OOP/inheritance paradigm, since it's just about providing some more props to the component, it's behavior is otherwise untouched. Good point about doing the wiring in |
Not sure what you mean by "two entry points". What I'm picturing is something like this: import {selectCommonProps} from "app/commonSelectors";
import {selectA, selectB} from "./specificSelectors";
const mapState = (state) => {
const propA = selectA(state);
const propB = selectB(state);
const commonProps = selectCommonProps(state);
return {a, b, ...commonProps};
} |
@markerikson By two entry points I meant that you'd have to do the same for |
You should almost never be using import {addTodo, toggleTodo} from "./todoActions";
class TodoList extends Component {}
const actions = {addTodo, toggleTodo};
export default connect(mapState, actions)(TodoList); You could easily have some index.js file that re-exports all your "common" action creators, and do something like: import * as commonActions from "app/common/commonActions";
import {specificAction1, specificAction2} from "./actions";
const actionCreators = {specificAction1, specificAction2, ...commonActions};
export default connect(null, actionCreators)(MyComponent); |
Hello @markerikson, I'm just curious about why someone should avoid using mergeProps? I find it very convenient to "hide" props from the mapStateToProps that I could need in my actions in mapDispatchToProps but not in the component. Is it a bad thing? |
Hi @gaearon - the following statement caught me off guard while reading the docs:
Deep prop chains was one of the issues with React that led me to use Flux; the tradeoff for making it easier to trace the data flow is that you need to setup/maintain/trace the prop flow, and more importantly, parents need to know about the data requirements of their children. From my experience, I'm not convinced that this approach is better, but I'm curious to hear what you think 😄
The text was updated successfully, but these errors were encountered: