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

Allow defining store mixins via a spec syntax similar to Component mixins. #37

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

STRML
Copy link
Contributor

@STRML STRML commented Jun 28, 2014

Fixes #32.

I found that I was defining similar functionality on a number of stores, especially if they listened to the same data.

Similar to React components, this will merge actions, chain functions, and attempt to merge objects/arrays. It will not, however, merge the result of functions (like createMergedResultFunction) as we have no such use case.

All the mixin functions are assigned directly to createStore so they can be overridden by users if necessary; for instance, I could see users wanting to define a different merge policy, such as deep merge.

@STRML
Copy link
Contributor Author

STRML commented Jun 28, 2014

If you are good with this syntax, I would be happy to add to site/contents/documentation/stores.md.

@BinaryMuse
Copy link
Owner

Hey, @STRML, thanks! This is cool stuff. I'm going to tag this for the 2.0 release for now, as there are a couple pending decisions around the 2.0 API I'm working through and I want this to be part of that thought process.

@BinaryMuse BinaryMuse added this to the 2.0 milestone Jun 29, 2014
@STRML STRML force-pushed the storeMixins branch 2 times, most recently from 5f35b2b to b76ac74 Compare September 12, 2014 22:11
@STRML
Copy link
Contributor Author

STRML commented Sep 12, 2014

Went ahead and rebased this to master. Just wanted to let you know I've been using it for about 4mo, 2 of those in production, and it's working pretty well.

I've been using a combination of mixins like this:

mixins: [
  AutoRefreshMixin({
    subscriptionName: 'trade',
    storeKey: 'trades'
  }),
  RestBindingMixin({
    storeKey: 'trades',
    restEndpoint: '/trade/getRecent',
    restParams: {
      count: 100
    }
  })
]

It's been hugely valuable in my workflow - my stores have auto-generated fetch methods and can even automatically connect to realtime data via a mux-demux stream. This would be much more difficult to do without mixins.

@evindor
Copy link

evindor commented Feb 5, 2015

This is awesome, mixins are a must for stores! +1!

@evindor
Copy link

evindor commented Feb 6, 2015

@STRML I just merged this against current master in my fork and I had to use component.bindActions instead of merge here: https://github.com/BinaryMuse/fluxxor/pull/37/files#diff-32584de7a758f3cebf1bd66000a8b7caR47

I think .bindActions should do the job of merging or am I missing something? @STRML you've put .merge here for a reason, right?

@STRML
Copy link
Contributor Author

STRML commented Feb 6, 2015

@evindor .bindActions may indeed work, in this case I thought it made more sense to simply merge the object and let Fluxxor call bindActions when initializing the store. I am not sure why it is not working for you; which version of Fluxxor did you merge this to?

@evindor
Copy link

evindor commented Feb 6, 2015

@STRML merged to current master.

@evindor
Copy link

evindor commented Feb 6, 2015

@STRML Seems like Fluxxor just never calls .bindActions anywhere else. Anyway it works for me now. Hope we'll get it merged soon!

@STRML
Copy link
Contributor Author

STRML commented Feb 6, 2015

@evindor I've rebased to latest master and it passes tests again.

When I wrote this PR, around version 1.4, createStore didn't call .bindActions but instead just merged into the __actions__ object. Around 1.5 it changed the behavior. This PR now uses the new behavior so it should work just fine.

@BinaryMuse
Copy link
Owner

Just wanted to pop in for a quick update here.

There's no doubt that this functionality is quite useful from a reusable abstractions standpoint, but I've always been hesitant to merge it in due to the complexity that mixins bring (similar to the complexity they brought to React). As you probably know, there's been a big push toward native ES6 classes from the React team, and using native classes in React 0.13 precludes the use of mixins (as provided by React.createClass), with the goal of utilizing more idiomatic JS patterns to implement that functionality in the future (or using composition instead).

Furthermore, a future version of Fluxxor is likely to provide a more flexible method of store creation, which will include less "magic," and I'm not sure how this particular feature will fit in to that effort. So, for now, I'm going to leave this unmerged, but rest assured that making reusable store patterns easier to implement is definitely on my mind going forward.

if (!_isObject(spec[key])) {
throw new Error("Actions must be defined as an object.");
}
component.bindActions(spec[key]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just realized, we need to chain these, if both a Store and its mixins are listening to the same action, only the Store will fire the listener.

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

Successfully merging this pull request may close these issues.

Store creation is too inflexible
3 participants