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

consider always passing down dispatch regardless of what mapDispatchToProps does #145

Closed
jdefontes opened this issue Oct 9, 2015 · 14 comments

Comments

@jdefontes
Copy link

I'm experimenting with composing several higher-order-components, like so:

compose(
  connect(mapStateToProps),
  authorize(unauthorizedAction),
  loadData(loadDataAction)
)(MyComponent);

It would be really nice if I could count on connect-ed components always having dispatch defined. Because if I do this:

compose(
  connect(mapStateToProps, { someAction, anotherAction }),
  authorize(unauthorizedAction),
  loadData(loadDataAction)
)(MyComponent);

...then my dispatch goes away in the connected component, and I have to implement a much more awkward mapDispatchToProps in order to get it back.

It's worth noting that redux-form also suffers from this problem.

IMO, being able to rely on some consistent props in a connected component would make this kind of third-party composition a lot easier.

@gaearon
Copy link
Contributor

gaearon commented Oct 9, 2015

Can you elaborate on why you need dispatch after binding action creators?
Usually components want it one way or the other way, or there's potential for confusion.

@jdefontes
Copy link
Author

Mostly because it's cleaner to have the sub-components know what actions they want by reference, rather than by name. I could do this:

compose(
  connect(mapStateToProps, { unauthorizedAction, loadDataAction, someAction, anotherAction }),
  authorize('unauthorizedAction'),
  loadData('loadDataAction')
)(MyComponent);

...but then I've got duplication and magic strings. My real use case is that the sub-components might want a map of several actions, so they can't just look for a well-known name in the parent props.

It also makes it clearer which actions are used by which HOCs.

@gaearon
Copy link
Contributor

gaearon commented Oct 9, 2015

Can you give me some idea as to how e.g. authorize looks?
What would authorize(unauthorizedAction) do?

@jdefontes
Copy link
Author

I thought about this a lot over the weekend and realized that I made the classic mistake of prescribing the solution rather than describing the problem.

I think there is the potential for different kinds of useful higher-order-components that would have some or all of the following characteristics:

  • they need to hook something in the React component lifecycle, so they want to be HOCs vs. some other kind of composition
  • they have some internal state that they maintain in the Redux store
  • they dispatch their own actions for managing that state, some of which may also be part of their public API
  • they act on some external state that has to be passed in by the application
  • they encapsulate or coordinate some behavior which requires external actions to be passed in by the application

redux-form is one example: it maintains the intermediate form state in the store; it exposes public actions for initializing/reseting the form; it depends on external actions for validation/form submission/etc.

My internal "authorize" HOC would: maintain the set of roles assigned to the current user and only render the wrapped component if the user was authorized; expose actions for intializing/updating the assigned roles; execute some application-dependent behavior on sucess/failue (maybe present a dialog for the user to request permission, or sign them out, or whatever the app wants to do).

I'm also thinking about an "ensureData" HOC that would ensure that the remote data required by a component was loaded, and manage the client-side caching of that data. It would maintain some internal bookkeeping information, expose actions for invalidating the cache, and would depend on external actions for actually loading the data.

So my question becomes: when a component wants to compose several of these concerns, what is the best way to get them the Redux bits that they need?

One approach is that there is a single connect()-ed component at the top of the chain, and all the dependencies required further down would be threaded through a single connect() call. That can be kind of awkward even for a single component (see: Doing the connect()ing Yourself for redux-form). I think that approach would become increasingly messy as you compose additional concerns.

redux-form works around this by providing its own wrapper around connect() to ensure that it gets its state from the store and a handle so it can dispatch() its own actions. I was a little puzzled when I first saw that, because I knew the client of the form would also need stuff from the store, and would also have to dispatch, so you'd wind up having a component structure like:

<Connect>
  <MyFormWrapper>
    <Connect>
      <MyForm />
    </Connect>
  </MyFormWrapper>
</Connect>

The nested <Connect>s seemed odd to me.

But the more I think about it, the more I'm coming around to the idea that that is the best way to achieve separation of concerns. Each HOC has it's own relationship with the store, and trying to roll up multiple components' needs into a single connect() call seems to be mixing up things that should be separate.

This makes my hypothetical example look like:

<Connect>
  <Authorize>
    <Connect>
      <EnsureData>
        <Connect>
          <MyFormWrapper>
            <Connect>
                <MyForm />
            </Connect>
          </MyFormWrapper>
        </Connect>
      </EnsureData>
    </Connect>
  </Authorize>
</Connect>

...which looks a bit crazy. But maybe that is just the world we live in when we compose with HOCs rather than mixins?

@gaearon
Copy link
Contributor

gaearon commented Oct 14, 2015

@erikras Thoughts?

@erikras
Copy link
Contributor

erikras commented Oct 14, 2015

I have certainly noticed that, as my application grows, I have many connected components nested inside other connected components, as @jdefontes has rendered in his last comment. I have been operating under the assumption that the redux listener system is probably robust enough to notify all the elements when a change occurs.

The way my pre-flux isomorphic React app was structured involved passing down all the data from the initial route component all the way to the farthest leaves, which gets disgusting very quickly, and avoiding that via state stored in the context is one of the biggest benefits of the Flux architecture.

I agree with @jdefontes' conclusion:

Each HOC has it's own relationship with the store, and trying to roll up multiple components' needs into a single connect() call seems to be mixing up things that should be separate.

@gaearon
Copy link
Contributor

gaearon commented Oct 15, 2015

Shall we close then?

@gaearon
Copy link
Contributor

gaearon commented Oct 15, 2015

I'm closing but let me know if you're not satisfied with nested smart components.

@gaearon gaearon closed this as completed Oct 15, 2015
@jdefontes
Copy link
Author

Thanks for the discussion.

Just one further observation... if you try to force a tree of HOCs to share a top level <Connect> then the whole tree gets notified when any of the various HOCs state changes, which becomes pathological in the case of, say, redux-form, where every keystroke updates the store. When you nest <Connect>s, each one of them gets separately notified, but they have the very nice property of acting as a guard for their children, and only updating the children if the state they care about has changed. So I see that as another vote for nested <Connect>s.

I agree with @erikras that, even setting aside the question of HOCs, it is just too damn convenient to not have a few nested <Connects> in a real app. Threading props through a deep component tree is a PITA and doesn't separate concerns very well. The previous version of my app using vanilla Flux had nested components that individually subscribed to the stores they cared about, so nesting <Connect>s is the functional equivalent of that for me. Viva Redux! (and redux-form is awesome too! ;-)

@ghost
Copy link

ghost commented Oct 25, 2015

After my exploration of redux and its advantages / tradeoffs I have concluded that this is the one piece I strongly dislike (as it applies to React): no convenient / modular way to nest higher order components (HOC).

Most apps I build consist of numerous HOCs with demanding (i.e. complex) business rules that will require their own reducer because a parent is incapable of determining its state transformation needs. You certainly don't need to connect() each component that requires a reducer, but you might need to connect them for the purposes of selection. Simply put...

let Parent = React.createClass({
    return (
        <div>
            <Child />
        </div>
    )
})

let ParentReducer = (state, action) => {
    return {
        child: process(state) // do some processing
    }
}

let ParentSelector = (state={ child: '' }) => {
    return state
}

let connected = connect(ParentSelector)(Parent)

... won't work unless I hand down the state to the child explicitly in the view via <Child state={this.props.child}/>. This requirement isn't that bad, since it's kind of the foundation of pass-down props defined by React. What stinks is when <Child /> also has <Grandchild />. Now I'm trapped in this highly-coupled pass-down party®.

<Parent>
  <Child state={this.props.child}>
    <Grandchild state={this.props.child.grandchild} />
  </Child>
</Parent>

To be fair, I suppose this could be alleviated by avoiding doubly-nested components and instead using a grandchildReducer and maintaining that only a <Child /> "knows" a <Grandchild />.

let childReducer = (state, action) => {
  return {
    childProps: { ... }
    , grandchild: grandchildReducer(state, action)
  } 
}

let grandchildReducer = (state, action) => {
  return {
    ...state // do some processing
  } 
}

<Parent>
  <Child state={this.props.child} />
</Parent>

<Child>
  <Grandchild state={this.props.grandchild} />
</Child>

For the sake of argument, what if we connected each HOC? That would avoid the prop pass-down problem, though I fear it would introduce another issue (that might be worse).

As mentioned by @jdefontes, authentication is a big piece of any app. Normally I write applications to be "secured" via different containers entirely for public or secure inner applications, where an intercepted statusCode 401 moves people from secure back out to public.

<Connect>
    <App>
        <Connect>
            <Secure>
                <Connect>
                    <Root>
                    </Root>
                </Connect>
            </Secure>
        </Connect>
    </App>
</Connect>

The selector for each of these components becomes immediately annoying, fragile, and error-prone.

{
    app: {
        secure: {
            root: {
                lotsOfRootState: ''
                , ... furtherNestedHOCs
            }
            , someSecureState: ''
        }
        , public: {
            login: {
                someLoginState: ''
            }
            , somePublicState: ''
        }
    }
    , someGlobalState: ''
}

let rootSelector = (state) => {
    return {
        ...app.secure.root
    }
}

let Root = connect(rootSelector)(root)

Now, luckily the atom for these pieces can be flat, since there's no real state sharing between a secure / public app or the root. We could even use a special "global" state indicator for those key pieces of app knowledge.

{
    _meta: {
        authenticationLevel: 'secure'
    }
    , root: { }
    , secure: { }
    , public: { } 
}

mapStateToPropsSecure = (state) => {
    return {
        ...state.meta
    }
}

mapStateToPropsRoot = (state) => {
    return {
        ...state.root
    }
}

The problem arises when you get into nesting of related components that can't be flattened. Perhaps an array of group of categories with an array of items belonging to a group.

<Connect>
    <Root>
        <Connect>
            <GroupManager>
                <Connect>
                    <Group>
                        <Item />
                        <Item />
                        <Item />
                        <Item />
                        <Item />
                    </Group>
                </Connect>
                <Connect>
                    <Group>
                        <Item />
                        <Item />
                        <Item />
                    </Group>
                </Connect>
            </GroupManager>
        </Connect>
    </Root>
</Connect>

The reducers would look like the following:

let groupReducer = (state, action) => {
    // ...
    // do some state processing
    // ...

    return {
          someGroupState: ''
          , items: state.items.map((item) => itemReducer(item, action))
    }
}

let itemReducer = (state, action) => {
    // ...
    // do some state processing
    // ...

    return {
          itemState: ''
          , moreItemState: ''
    }
}

This means that an <Item /> would become a connect(itemSelector)(Item) component that extracts necessary state from the atom. But can we even do this? How would we know which item to pick from the list of items, assuming that list even exists at all in the early stages of an app's lifecycle.

let itemSelector = (state) => {
  return { state.root.groupmanager.group[???].items[???] } // we just can't do this
}

This just isn't possible. There is a limit to the depth inside the atom at which you can connect, and that limit starts with anything created at runtime.

Let's take a step back and reexamine the prop pass-down strategy. Another annoyance is that only connected apps receive a dispatch. This is good for keeping knowledge of redux away from the leaves of an app, but is also a lot of boilerplate (and potential coupling). How does the <Grandchild /> create an action? Well, it invokes the callback that was provided to it from the <Child />, which itself received the callback from the <Parent />, which declared the callback as cb = (value) => this.props.dispatch(someAction(value)). This also (kind of?) couples the parent to the grandchild because the parent has to know that the grandchild might want to doSomeAction, and someAction might be a separate piece of the domain. Consider onChildSelected versus onGrandchildSelected. Should the parent really have to know the domain of the grandchild in the sense of <Child onChildSelected={() => this.props.dispatch(childSelected)} onGrandchildSelected={() => this.props.dispatch(grandchildSelected)} />?

I would appreciate anyone else's thoughts on this matter. Is the "cleanest" solution simply a prop pass-down as demonstrated above? Is there a way around all the boilerplate? How much nesting can / should we do in the atom?

@idibidiart
Copy link

@gaearon

I fiund thus question by you (on S.O.) and another one by @ide (i just commented on that too so i apologize for the inadvertent loop) grappling with the effecf of this same exact issue, which was not satisfied IMO with the batchedupdate answer (you are still going to have overhead that grows with the complexity of the component tree)

reduxjs/redux#125

http://stackoverflow.com/questions/25701168/at-what-nesting-level-should-components-read-entities-from-stores-in-flux

I think the answer on S.O. is not ideal... leaving it to the programmer is not a comforting solution

have you any updated insight?

@gaearon
Copy link
Contributor

gaearon commented Jan 13, 2016

I don't understand what exactly you are asking. Please create a new SO question and link to it?

@idibidiart
Copy link

My question is it seems that both in this thread and on S.O. there are two points of view on how to compose components with Redux.

  1. Have a connector at the root of the component tree with state being pushed down to descendants as props and all descendants being pure stateless components
  2. All components in a component tree are stateful and have a connector

I understand from the S.O. thread that there are tradeoffs and benefits to each of the above approaches but I don't understand what the ideal way would be. Is it 1 or 2 or a combination of both as suggested in the accepted S.O. answer? Or is it some other model. I'm trying to understand how best to compose components when using Redux.

Your analysis in that S.O. post is very clear, and you seem to have been looking for the right solution but it's not clear what that is, and more importantly how composition should be done in Redux.

@gaearon
Copy link
Contributor

gaearon commented Jan 13, 2016

Both approaches are extreme. We suggest taking the middle approach.

Try to create presentational components and use them when possible. However when you feel like you're passing data through components in the middle that don't use it, it's a good signal that it's time to create a container component for that presentational component.

So basically create containers when you feel like some particular component has become hard to reuse because it requires too much unrelated data that parent components don't care about.

I strongly suggest you to watch the last 10 videos of my course because they deal with this particular question: https://egghead.io/series/getting-started-with-redux

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

No branches or pull requests

4 participants