acdlite / flummox

Minimal, isomorphic Flux.
http://acdlite.github.io/flummox
1.69k stars 114 forks source link

connectToStores: Enable using both Array and Object for defining stores #233

Closed bamorim closed 9 years ago

bamorim commented 9 years ago

Hi everyone. I don't know If this is already possible, but looking at the source code doesnt look like it is. I just think that It's a nice to have addition. (being able to use all kinds of getters :D)

I have a component where I need to get 3 values from stores with the same name (as used in the stateGetterMap) but for only one store i need to call a getter method on my store. I wanted to do:

export default connectToStores(
  HomeHandler,
  ["themes", "guides", "defaultGuides"],
  {                                                                                                                                                                                            
    disciplines: (store) => ({ disciplines: store.getDisciplines() })
  }
);

But I had to rewrite it to

export default connectToStores(
  HomeHandler,
  {
    themes: ({state: {themes}}) => ({themes})
    guides: ({state: {guides}}) => ({guides}),
    defaultGuides: ({state: {defaultGuides}}) => ({defaultGuides}),
    disciplines: (store) => ({ disciplines: store.getDisciplines() })
  }
);

Which is a little bit more verbose. What do you think?

johanneslumpe commented 9 years ago

This behavior would clash with the default state getter - which is what is being used when passing an array of store names. What you are passing in are store names, not properties on the state of a store with the same name. If you only pass in the store name, what you will get is the whole state of that store.

There wouldn't be a way to distinguish whether users want the behavior you are proposing or the current behavior of just receiving the full store state.

If you are going for the full store state, then your first example can be written like this:

export default connectToStores(
  HomeHandler,
  {
    themes: null
    guides: null,
    defaultGuides: null,
    disciplines: (store) => ({ disciplines: store.getDisciplines() })
  }
);

If you pass a value which is not a function as a state getter, it will be ignored and the default state getter will be used. So it becomes a lot less verbose - actually it is only 6 characters more than the original version (excluding spaces and new lines ;)) 141 to 147. Those 6 characters seem negligible to me.

Is that something you can work with?

bamorim commented 9 years ago

@johanneslumpe Nice to know!

What I did instead was override the getStateAsObject() and it worked fine, as the only reason to use a getter method was to index disciplines by id. Anyway, thank you. I think it can be closed now. But it'd be nice to have this in the docs. Also, a page only to the connectToStore method would be nice, and showing all forms of calling it (Mixin, Component and HOC).

I'll close this, sorry for bothering you. :smile:

bamorim commented 9 years ago

If you think it's valid, I can work on it, but as you said, that version (passing null) seems okay. Maybe something nice is to pass what properties you want instead of a function, like:

export default connectToStores(
  HomeHandler,
  {
    themes: ["themes"]
    guides: ["guides"],
    defaultGuides: "*", // Maybe a special character to import all?
    disciplines: (store) => ({ disciplines: store.getDisciplines() })
  }
);

What do you think?

johanneslumpe commented 9 years ago

/cc @acdlite