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

Thoughts on Store mixins #4

Closed
lelandrichardson opened this issue Jan 20, 2015 · 4 comments
Closed

Thoughts on Store mixins #4

lelandrichardson opened this issue Jan 20, 2015 · 4 comments

Comments

@lelandrichardson
Copy link

First, I'd like to say thanks for creating ReactFlux. I did a lot of looking around at Flux libraries to reduce some of the boilerplate, and I landed on ReactFlux as I think the general style agrees with me the most!

As I've been playing around with it, I've been creating some mixins for my Stores. Initially, I was doing it this way:

var MyStore = Flux.createStore(Object.assign({},
FooMixin,
BarMixin,
{
    ...
},[
    ...
]);

Then, after looking at the source, I found I was able to do this instead:

var MyStore = Flux.createStore({
    ...
},[
    ...
]);
MyStore._mixin(FooMixin);
MyStore._mixin(BarMixin);

Although, based on the method naming, it seems like this is using what is intended to be a private method.

Since Flux is likely to be used with React, the style I'm really looking for would be something like:

var MyStore = Flux.createStore({
    mixins: [
        FooMixin,
        BarMixin,
    ],
    ...
},[
    ...
]);

Is this syntax that you'd be open to? If so, I may submit it as a pull request.

Let me know, thanks!

@kjda
Copy link
Owner

kjda commented Jan 20, 2015

Hi Leland, thanks for your interest.
yes, this would be a good addition!

_mixin is intended to be private. If we want to provide a public API for adding mixins, then we should change the naming some how, because stores have "mixin" method for creating a ReactComponent Mixin.

@lelandrichardson
Copy link
Author

@kjda right. I knew the _mixin naming was at least in part due to the conflicting .mixin() method.

How do you feel about my proposal above? This would make it so that mixins could only be defined when defining the store itself using the Flux.createStore() factory method, and also has the benefit of following the same conventions as React.createClass()?

This would only require a special handling of any array-like mixins property in the private _mixin method. In fact, it would be easiest to simply make this method recursive. This would allow a mixins to compose on top of eachother:

var FooMixin = {
    bar: function () { ... }
};

var EnhancedFooMixin = {
    mixins: [FooMixin],
    doMore: function () { ... this.bar(); ... }
};

Flux.createStore({
    mixins: [EnhancedFooMixin],
    ...
},[
    ...
]);

What do you think?

@kjda
Copy link
Owner

kjda commented Jan 21, 2015

sounds great! recursive is also a good idea! Let me know if you want to do it.. if not, I can do it when I have some time.

@lelandrichardson
Copy link
Author

I'd be happy to do it. Let me see what I can do tonight and I'll submit a PR

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

2 participants