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

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

Closed
wants to merge 7 commits into
base: master
from

Conversation

10 participants
@skevy
Copy link
Contributor

skevy commented Jun 13, 2015

A possible solution to #84

@skevy skevy changed the title Allow for passing of actionCreators through `Connector` and `@connect` decorator Allow for passing of action creators through `Connector` and `@connect` decorator Jun 13, 2015

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 13, 2015

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

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 13, 2015

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

This comment has been minimized.

Copy link
Collaborator

acdlite commented Jun 13, 2015

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

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 13, 2015

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

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 13, 2015

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

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 13, 2015

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..

@@ -62,12 +68,27 @@ export default function createConnector(React) {
return { slice };
}

bindActionCreators(dispatch) {
if (!this.props.actionCreators) {
return {};

This comment has been minimized.

@gaearon

gaearon Jun 13, 2015

Contributor

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

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 13, 2015

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

This comment has been minimized.

Copy link
Contributor

skevy commented Jun 13, 2015

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

This comment has been minimized.

Copy link
Contributor

skevy commented Jun 13, 2015

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

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 13, 2015

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

This comment has been minimized.

Copy link
Contributor

skevy commented Jun 13, 2015

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

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 13, 2015

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

This comment has been minimized.

Copy link
Contributor

skevy commented Jun 13, 2015

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

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 13, 2015

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

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 13, 2015

(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

This comment has been minimized.

Copy link
Contributor

skevy commented Jun 13, 2015

Fair enough :)

@skevy

This comment has been minimized.

Copy link
Contributor

skevy commented Jun 14, 2015

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

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 14, 2015

When would that happen? Only in hot-reloading?

Yeah, I think so.

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 14, 2015

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

@skevy

This comment has been minimized.

Copy link
Contributor

skevy commented Jun 14, 2015

Absolutely. Was already working on it.

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 14, 2015

Thanks!

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 16, 2015

I would like to have some more eyes on this API-wise.
Is there anything we can do to make/keep the API simpler?

@ooflorent @acdlite @johanneslumpe?

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 16, 2015

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

This comment has been minimized.

Copy link
Contributor

dariocravero commented Jun 16, 2015

Perhaps select should be explicitly set at all times?

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 16, 2015

@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

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 16, 2015

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

@johanneslumpe

This comment has been minimized.

Copy link
Contributor

johanneslumpe commented Jun 16, 2015

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

This comment has been minimized.

Copy link
Contributor

dariocravero commented Jun 16, 2015

@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

This comment has been minimized.

Copy link
Contributor

johanneslumpe commented Jun 16, 2015

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

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 16, 2015

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

This comment has been minimized.

Copy link
Contributor

johanneslumpe commented Jun 16, 2015

@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

This comment has been minimized.

Copy link
Contributor

emmenko commented Jun 16, 2015

I like the decorator solution

@leoasis

This comment has been minimized.

Copy link
Contributor

leoasis commented Jun 16, 2015

👍 to the separate decorators and components

@tsingson

This comment has been minimized.

Copy link

tsingson commented Jun 16, 2015

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

This comment has been minimized.

Copy link
Contributor

skevy commented Jun 16, 2015

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

This comment has been minimized.

Copy link
Contributor

skevy commented Jun 16, 2015

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

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 16, 2015

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

This comment has been minimized.

Copy link
Contributor

skevy commented Jun 21, 2015

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

This comment has been minimized.

Copy link
Contributor

gaearon commented Jun 21, 2015

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

@gaearon gaearon referenced this pull request Jun 22, 2015

Closed

Umbrella 1.0 #164

13 of 13 tasks complete
@guillaume86

This comment has been minimized.

Copy link

guillaume86 commented Jul 5, 2015

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

This comment has been minimized.

Copy link

cef62 commented Jul 5, 2015

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

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Jul 11, 2015

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

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Jul 12, 2015

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

This comment has been minimized.

Copy link
Contributor

gaearon commented Jul 12, 2015

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

@gaearon gaearon closed this Jul 12, 2015

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Aug 7, 2015

@skevy

This comment has been minimized.

Copy link
Contributor

skevy commented Aug 7, 2015

Wooooo :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment