Closed acdlite closed 9 years ago
@tappleby Would love your thoughts on this
Action injection is pretty cool, IMO.
Yeah I think this could be a handy feature, especially with "dumb components" that have callbacks. I picture it having a similar api as connectToStores
.
Will need to be careful with the bound context for this
when using actionA: actions.actionA
.
Yes, good point. Since actions are created using a closure (https://github.com/acdlite/flummox/blob/master/src/Actions.js#L51-L63) this shouldn't be a problem with the current implementation, but we should write a test protect against regressions.
Actually it's the apply()
that really does that, not the closure
Bleh I guess the closure ensures this._dispatchAsync
and this._dispatch
still work :D
I would like to see this added as well, as it would (as I see it) remove the dependency on the flux prop else needed, when calling actions in the views.
Forgot the _wrapAction
method handled that!
This may need to be a separate issue, but we could also allow specifying an array of action method names, instead of a state getter function:
// Equivalent to first example
<FluxComponent
connectToStores="myStore"
injectActions=({
myActions: ['actionA', 'actionB'], // array of action method names
})
render={(storeState, actions, flux) => <ChildComponent {...actions} {...storeState} />}
/>
We could do the same for connectToStores()
, where the array contains store state keys.
That could be handy as well. Sort of what I was thinking with similar api as connectToStores
.
Would the structure of the actions
object passed to render handler match injectActions
?
eg:
{
myActions: {
actionA: ...,
actionB: ...
}
}
No it would work like connectToStores()
, where the values are collected into a shallow object:
{
actionA,
actionB
}
Thats what I was hoping.
My biggest use case for this would probably be the object syntax, mapping callbacks on UI (dumb) components to actions:
<FluxComponent
connectToStores={{
posts: (store, props) => ({
post: store.getPost(props.postId)
})
}}
injectActions=({
posts: actions => ({
onSave: actions.savePost,
onDelete: actions.deletePost,
})
})
render={(storeState, actions, flux) => <UIEditPost {...actions} {...storeState} />}
/>
Also if a callback's arguments doesn't map to an action, it can easily be wrapped:
injectActions=({
posts: actions => ({
onSave: actions.savePost,
onDelete: (post) => actions.deletePost(post.id),
})
})
@tappleby Great, that's exactly the idea behind the proposal. I'll probably copy and paste your example directly into the docs :D
:+1:
As already pointed out on IRC, just to confirm: This is a really great idea from my POV. And as @tappleby already said, it will help with the decoupling of components and lead to more reusable components, which just rely on callbacks, making them flux-framework agnostic.
Nice! Would be really useful.
love it!
+1 for this :)
@acdlite Seems there is support for this, let's make this happen :D
+1
This will be a breaking change (because of the changed params to the render method) so it will have to go in the next major release. @gaearon Do your little birdies have an idea of when React 0.14 will land? Not sure if it's worth waiting until then.
Do your little birdies have an idea of when React 0.14 will land? Not sure if it's worth waiting until then.
I heard 0.13-0.14 transition should take one or two months and certainly faster than 0.12-0.13 was. Counting on it landing soon.
Yeah I saw that they've already bumped the master branch to 0.14.0-alpha. In that case I think it's worth it to wait a bit longer so we can push all the breaking changes out at once.
yay!, break all the things!
+1
Can't wait for that ! +1
+1 Nice idea. This will allow most component code to avoid referencing this.(props.)flux
at all.
What's the status on this? I would like to use this right meow.
Excuse me...did you just say "meow"?
Do I look like a cat to you, boy? Am I jumpin' around all nimbly bimbly from tree to tree?
I think there must be something union for connectStores and injectActions without FluxComponent. And more like decorator:
class Component extends React.Component {
render() {
const {anotherAction} = this.props.actions;
const {someStore} = this.props.stores;
}
}
export default fluxify({
stores: ['someStore'],
actions: ['anotherAction']
})
I think @Aetet has a point. It might be nice to have 3 HOCs: connectToStores
, injectActions
and a third one which combines the two under the hood. even though it's not strictly necessary as we could just to connectToStores(injectActions(..), ...)
. But having to type that over and over again does warrant a third HOC which does both I guess.
Hmm, yeah I like the idea of breaking it up into multiple HoCs... I was just going to combine them all into one, but I like the composability of that approach.
...while still providing the combined HoC, as well
Also I think we should rename injectActions
and connectToStores
to just actions
and stores
. Simpler.
hmmm, maybe a middle ground like withActions
and withStores
? Just to keep some semantics.
Not only is the injectActions
terminology reminiscent of IoC/DI, but it's not exactly intuitive from connectToStores
which is is already there and presumably not going away. Why not just connectToActions
and maybe a third HoC connectTo
?
Edit: Actually I agree with @acdlite. If the API is changing, actions
and stores
would be a lot cleaner.
Is this a useful feature? Do people want this?