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

Shape of props depends on number of stores #22

Closed
fson opened this issue Jun 3, 2015 · 11 comments
Closed

Shape of props depends on number of stores #22

fson opened this issue Jun 3, 2015 · 11 comments

Comments

@fson
Copy link
Contributor

fson commented Jun 3, 2015

Changing the names of props based on number of stores subscribed to seems tricky.

Let's say I have a dumb component that takes a prop board ( { name: 'My Board' }), which should come from boardStore:

<Container stores={[boardStore]}
           actions={{ addList }}>
  {(props) => <Board {...props} /> }
</Container>

But because I only subscribed to one store, it gets flattened and the component only receives name prop.

If I need to add a subscription to another store userStore, the props change to:

{
  boardStore: { name: 'My Board' },
  userStore: ...,
  fooStore: ... // (This actually includes data from all stores, not just subscribed.)
}

I think it would be simplest, if the data would never be flattened. Then I can always just do:

<Container stores={[boardStore]}
           actions={{ addList }}>
  {(props) => <Board board={props.boardStore} /> }
</Container>

Another way could be to pass another function or a map of functions to map the state from stores to
props, but I think I actually prefer the former because of its simplicity.

@gaearon
Copy link
Contributor

gaearon commented Jun 3, 2015

Agreed, the current behavior is too implicit. I like where you're going with “never flatten” but you can't do the same for the decorator. I guess the decorators could accept a second transformProps argument to be on par with inside-render composition.

<Container stores={[boardStore]}
           actions={{ addList }}>
  {({ boardStore, ...props }) => <Board board={boardStore} {...props} /> }
</Container>
@container({
  stores: [boardStore],
  actions: { addList }
}, ({ boardStore, ...props }) => ({ board: boardStore, ...props }))
class Board {

Thoughts?

@fson
Copy link
Contributor Author

fson commented Jun 3, 2015

transformProps looks OK.

Another thing to consider is that ideally the container would filter the props
passed to the children function/transformProps and only pass the fragments
you subscribe to. (Same way as Relay only gives props that you have query for.)

Otherwise it's easy to accidentally pass props from stores you haven't subscribed
to, which works initially, but fails when you don't receive updates from those
stores.

@ooflorent
Copy link
Contributor

What about defining stores as object or as a function where we can return a state using restructuring? I'll try to submit a proposal tonight.

@hugooliveirad
Copy link

Agreed with @fson.

@ooflorent this goes against the whole purpose of stores being simple reducing functions (as far as I understood your proposal.) But, I agree we should have access to all data in Redux, maybe with a Redux.getState.

@gaearon
Copy link
Contributor

gaearon commented Jun 3, 2015

Here's something even more explicit. I think I like it:

<Container stores={[boardStore]}
           actions={{ addList }}>
  {({ actions, stores }) => <Board board={stores.boardStore} {...actions} /> }
</Container>

This works great with other possible usage patterns:

<Container stores={[boardStore]}
           actions={{ addList }}>
  {({ actions, stores }) => <Board {...stores} {...actions} />}
</Container>
<Container stores={[boardStore]}
           actions={{ addList }}>
  {({ actions, { boardStore } }) => <Board {...boardStore} {...actions} />}
</Container>

or even

<Container stores={[boardStore]}
           actions={{ addList }}>
  {({ actions, stores }) => <Board stores={stores} actions={actions} />}
</Container>

It's also good if you don't care about the actions (or the stores):

<Container actions={{ addList }}>
  {({ actions }) => <Board actions={...actions} />}
</Container>

If somebody wants to take shot at it, let me know here :-)

I'm busy with thinking about middleware at the moment, but I'm happy to merge the version I described above, with the addition of transformProps second argument to the decorator, and a fix for

Another thing to consider is that ideally the container would filter the props
passed to the children function/transformProps and only pass the fragments
you subscribe to. (Same way as Relay only gives props that you have query for.)

(which was a regression in 0.3.0 rewrite)

@gaearon
Copy link
Contributor

gaearon commented Jun 3, 2015

It might be that for consistency, we should change stores to accept an object:

<Container stores={{ boardStore }}
           actions={{ addList }}>
  {({ actions, { boardStore } }) => <Board {...boardStore} {...actions} />}
</Container>

Now, stores and actions are in symmetry. This also lets us specify custom prop names without messing around too much:

<Container stores={{ board: boardStore }}
           actions={{ addList }}>
  {({ actions, stores }) => <Board {...stores} {...actions} />}
</Container>

In the children callback, stores will be { board: ... } because that's how we specified it.
This also makes it easier for the decorator users:

@container({
  stores: { board: boardStore },
  actions: { addList }
  // by default transformProps is ({ stores, actions }) => { ...stores, ...actions }
});
@container({
  stores: { boardStore },
  actions: { addList }
}, ({ actions, stores }) => ({ board: stores.boardStore, ...actions })); // but can customize

@ooflorent
Copy link
Contributor

@gaearon your proposal is what I was thinking about. If no one submit a PR I'll do it tomorrow.

@gaearon
Copy link
Contributor

gaearon commented Jun 3, 2015

👍

@fson
Copy link
Contributor Author

fson commented Jun 3, 2015

@gaearon I really like that proposal, makes it very clear what is happening. PR coming in a moment.

@gaearon
Copy link
Contributor

gaearon commented Jun 4, 2015

This is out in 0.6.0.
Thank you @fson for raising this issue & for the extremely clear PR.

@fson
Copy link
Contributor Author

fson commented Jun 4, 2015

Thanks!

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