Allow for passing of action creators through Connector and @connect decorator

[skevy]

A possible solution to #84

[gaearon]

I like to have less boilerplate, but here's what I don't like about this API:

• @connect has just gotten harder to understand in my opinion because it no longer does single thing
• there is no way to implement efficient shouldComponentUpdate for the contained components because they'll get new bound functions every time

I can live with the first, but the second one is a red flag to me. If we're breaking shouldComponentUpdate of user's component, we're doing something wrong.

This is also a problem with the current API and I agree it needs fixing. I just want to add first-class action creator binding support if it also fixes the shouldComponentUpdate issue.

[gaearon]

One way to work around the shouldComponentUpdate problem is to keep the bound functions in the Connector's state and only rebind if:

• dispatch changed (which means Store got hot-reloaded)
• passed action creators are shallowly unequal (which means Actions got hot-reloaded)

[acdlite]

@gaearon Yes, I think that's the way to go.

[gaearon]

You can try exploring this API:

@connect((state, bindActionCreators) => ({
  counter: state.counter,
  ...bindActionCreators(CounterActions)
}))
export default class Counter {
  render() {
  }
}

function select(state, bindActionCreators) {
  return {
    todos: state.todos,
    actions: bindActionCreators(TodoActions)
  };
}

export default class TodoApp {
  render() {
    return (
      <Connector select={select}>
        {this.renderChild}
      </Connector>
    );
  }

  renderChild({ todos, actions }) {
    return (
      <div>
        <AddTodo {...actions} />
        <TodoList todos={todos} {...actions} />
      </div>
    );
  }
}

You'll need to figure out some way of memoizing the bound functions though, so that if the dispatch function is the same, and passed action creators are the same, they're not re-bound on each select call.

[gaearon]

What I like about select(state, bindActionCreators): one-stop shop for the Flux connect thing both for state and actions in one function. Easy to shape the resulting props the way you want to.

What I don't like: “smart” memoization logic it would need to be efficient. I'd rather have select(state, dispatch), but if bindActionCreators was a separate utility, there would be no efficient way for memoizing.

// I wish I could make this fast
function select(state, dispatch) {
  return {
    todos: state.todos,
    actions: bindActionCreators(TodoActions, dispatch)
  };
}

[gaearon]

Also, as for my last suggestion—it's actually not much better than what we have now already.
So we're back to what you proposed initially..

[gaearon]

I think a sane default is the current behavior: injecting just the dispatch prop. Let's remove this early exit, and instead specify dispatch => ({ dispatch }) as the default prop value for actionCreators.

[gaearon]

What is the use case for actionCreators-as-a-function? Do we need it? It seems to me that middleware already allows dispatch-level customization, so at this point I'd rather have actionCreators strictly be an object.

It's easy to pick just what you want for it:

import { increment } from './CounterActions';
import * as TodoActions from './TodoActions';

<Connector actionCreators={{ increment, ...TodoActions }}>

[skevy]

I was thinking of namespaced actions when I thought of actionCreators as a function...something like:

import * as CounterActions from './CounterActions';
import * as TodoActions from './TodoActions';

<Connector actionCreators={(dispatch) => ({
    counter: bindActionCreators(CounterActions, dispatch),
    todo: bindActionCreators(TodoActions, dispatch)
}) />

Anyway, of course this suffers from that shouldComponentUpdate problem as well, and complicates things. It's not really necessary.

[skevy]

Also, as for my last suggestion—it's actually not much better than what we have now already.
So we're back to what you proposed initially..

Were you referring to having the select function combine the actions and other props...or were you talking about #86 (comment)?

[gaearon]

Both. I think burdening select with this is not much better than doing it in render in the first place..
It makes select too magic because of the memoization trickery it would need.

I'm starting to like your original suggestion, given that you add some memoization, and remove the function overload. I think that, instead of namespacing, it's better to merge the actions, and if there are conflicts, you can always assemble a custom object and rename the functions as you need.

[skevy]

Talk to me a little bit about how hot loader works with decorators right now...it reinitializes the whole component on every change, am I right?

[gaearon]

Talk to me a little bit about how hot loader works with decorators right now...it reinitializes the whole component on every change, am I right?

Yeah, that's the problem with decorators. I'll solve it eventually with this plugin but for now I recommend using container components for that very reason.

[skevy]

As I go through this memoization process...I'm noticing this potential problem:

<Connector actionCreators={{ ...CounterActions, ...TodoActions }}>
{() => [things]}
</Connector>

That's going to create a new object every render()...thus negating any prop checking in Connector and causing a rebind.

It would require the user to implement storing these elsewhere...which is completely fine...I think we just need to be sure we mention this somewhere.

[gaearon]

That's going to create a new object every render()...thus negating any prop checking in Connector and causing a rebind.

I'd say it's likely that the developer would measure where shouldComponentUpdate is failing if they have a performance problem, and notice that it's due to this object, and figure out to move it outside render.

const actionCreators = { ...CounterActions, ...TodoActions };

<Connector actionCreators={actionCreators}>
  {() => [things]}
</Connector>

I think we can write an “Optimizing” doc later, but it's too early at this point as we're still figuring out the API.

[gaearon]

(The same argument can be made for select. It's possible to write it as a lambda, and have potential perf problems because of that, but it would catch the eye of seasoned React dev looking for performance problems, so we're just like the other libraries in this regard.)

[skevy]

Fair enough :)

[skevy]

Just pushed a change the saves the bound action creators and only updates them when passed action creators changed. I know you also mentioned making sure they get updated when the dispatch method changes, which if I'm not mistaken, only happens when the entire redux instance is re-created, am I right? When would that happen? Only in hot-reloading?

[gaearon]

When would that happen? Only in hot-reloading?

Yeah, I think so.

[gaearon]

Great. Can I ask you to update the examples and README to show how this API makes Redux better?

[skevy]

Absolutely. Was already working on it.

[gaearon]

Thanks!

[gaearon]

What about the case where a component only needs to fire actions, but doesn't care about stores? The default select function would then have it to subscribe to every single root atom change. This doesn't seem good.

[dariocravero]

Perhaps select should be explicitly set at all times?

[gaearon]

@dariocravero

But what would you specify in case you only want the actions? select={() => ({})}? Sounds like working around the problem.

Here's another proposal: use a special component for binding actions.

Decorator version

@bind(CounterActions)
@connect(state => ({
  counter: state.counter
}))
export default class CounterApp {
  render() {
    return (
      <Counter {...this.props} />
    );
  }
}

Container version

export default class TodoApp {
  render() {
    return (
      <Bind actionCreators={TodoActions}>{(actionCreators) =>
        <Connect select={state => ({ todos: state.todos })}>{({ todos }) =>
          <div>
            <AddTodo {...actionCreators} />
            <TodoList todos={todos} {...actionCreators} />
          </div>
        }</Connect>
      }</Bind>
    );
  }
}

It would do a single thing, it would not complicate the existing @connect API by introducing a second parameter, and it would be usable directly without <Connect> if the user doesn't need to observe state, but needs to fire actions in a particular component.

Thoughts?

[gaearon]

Don't forget the user can always create their own <Container> component that unifies <Bind> and <Connect> if they wish so.

[johanneslumpe]

I think that we definitely need a way to allow a component only to dispatch actions but to not get notified by store changes. Of course it's very useful for us lazy people that we can just use @connect without a select function and get all the state automatically injected, but it as soon as this brings performance-implications with it, that's not great.

What about if connect were to work like this:

@connect({
 select: (state) => {...},
 actions: actionObject
})

The actionObject could automatically be spread as props onto the wrapped component and this would allow users to specify whether they want to select data or not. I would rather just pass actions verbatim, without binding them, too.

[dariocravero]

@gaearon the fact that state isn't set should be enough, shouldn't it?
@johanneslumpe like flummox :)
@Gaeron's approach of having two decorators/components is probably a good thing anyway to separate responsibilities and minimise the impact

[johanneslumpe]

That would work, yeah. I'm just not very keep on binding the actions, as it introduces some overhead. But since that stuff is optional anyway...

[gaearon]

What about if connect were to work like this:

That's how it used to be but I really dig the simplicity of a single function parameter. I feel like having select and actions in one place conflates two different needs: a real need for subscribing to updates, and a cosmetic need to bind actions. I'd rather have separate unopinionated components, and maybe some kind of <Container> that combines them if people want ready solutions.

[johanneslumpe]

@gaearon fair enough. Maybe instead of bind it could be named injectBoundActions or something like that? It would be nice if the name would reflect both the things this decorator does.

[emmenko]

I like the decorator solution

[leoasis]

👍 to the separate decorators and components

[tsingson]

I like the decorator solution too, and ,

@connect({
 select: (state) => {...},
 actions: actionObject
})

is good and clear.
look like connect is automatic bridge between actions and stores for component.

the render function in the `decorator' version:

export default class CounterApp {
  render() {
    return (
      <Counter {...this.props} />
    );
  }
}

is more familiar.

[skevy]

To those of you who are saying not to bind the actions...why not? The overhead is muted by the fact that we're memoizing the bind operation unless 1) the actions change or 2) the redux instance changes.

[skevy]

As far as the multiple decorator/component approach, I'm down with that. I think having the <Container> would be useful as well...but perhaps unnecessary.

One problem that I've run into when using multiple decorators on a component is that if I need access to the ref of the component I'm decorating, many decorators makes that difficult to do (without touching internal APIs). So if we just keep adding decorators upon decorators (as awesome as they are)...it can break component-to-component communication (a good use case for this, before everyone tells me I'm an idiot and shouldn't access components this way...is animation, where an inner component knows how to tween itself, but the parent container component actually handles synchronizing the tweens). It's very possible this doesn't matter...and there may be a BETTER way to handle this type of use case...but just thought I'd bring it up.

I can work later on the multiple decorator approach, if that's the direction we want to explore.

[gaearon]

I'd like other collaborators to express their thoughts here and give it some time to rest.
I'll get back to re-evaluating after my current time travel work.

[skevy]

thought I'd ping this to see if any of the other collabs have had time to review? I'm not particularly attached to my idea over anyone else's, but it would be cool to get something like the ideas presented merged before 1.0

[gaearon]

I'll definitely consider this before 1.0. :-)

[guillaume86]

I'm all for #86 (comment)
Before finding this thread I was thinking actions could be bound at the Provider level and retrieved with some decorator in child components but this is even better.

[cef62]

I give my +1 to the decorator solution, could be really useful!

[gaearon]

I'd love to hear more proposals for the alternative to the current <Provider> and <Connector> API: reduxjs/react-redux#1

[gaearon]

Here's a more radical variation on this, with a very similar API but slightly different prescribed usage.
It also completely drops <Connector> in favor of connect(): reduxjs/react-redux#1 (comment)

I'd appreciate your comments and ideas there.

[gaearon]

I'm closing this in favor of reduxjs/react-redux#1 (comment).
Let me know if you'd like to implement it!

[gaearon]

This is done in https://github.com/gaearon/react-redux/releases/tag/v0.5.0.

[skevy]

Wooooo :)