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

adding the listenables prop to createStore #63

Closed
wants to merge 38 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@krawaller
Contributor

krawaller commented Sep 10, 2014

This adds a convenient listenables prop to createStore, which lets you easily listen to all listenables in an object (read: the return value from createActions).

Here's the example added to the README. If you've previously done this:

var actions = Reflux.createActions(["fireBall","magicMissile"]);

...then instead of doing this:

var Store = Reflux.createStore({
    init: function() {
        this.listenTo(actions.fireBall,this.fireBall);
        this.listenTo(actions.magicMissile,this.magicMissile);
    },
    fireBall: function(){
        // whoooosh!
    },
    magicMissile: function(){
        // bzzzzapp!
    }
});

...you can now do this:

var Store = Reflux.createStore({
    listenables: actions,
    fireBall: function(){
        // whoooosh!
    },
    magicMissile: function(){
        // bzzzzapp!
    }
});

To enable this to be used with an object of stores too (hence the agnostic name listenables), I added support for getDefaultData in the listenable. That data is sent to the listening callback, or a this.<callbackname>Default if that exists.

@spoike

View changes

Show outdated Hide outdated src/createStore.js
@spoike

View changes

Show outdated Hide outdated src/createStore.js
@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Sep 10, 2014

Member

Looks like a nice idea. 👍

Though it would probably make more sense if the methods are prefixed with 'on' and capitalize first letter. E.g.

var actions = Reflux.createActions(["fireBall","magicMissile"]);

var Store = Reflux.createStore({
    listenables: actions,
    onFireBall: function(){
        // whoooosh!
    },
    onMagicMissile: function(){
        // bzzzzapp!
    }
});

As a line comment here is a sample example of how things usually turn out with the action and store relationship:

var actions = Reflux.createActions(["fireBall","magicMissile"]);

// only needs to listen to magicMissile action
var MagicMissileStore = Reflux.createStore({
    listenables: actions,
    onMagicMissile: function(){
        // bzzzzapp!
    }
});

// only needs to listen to fireBall action
var FireBallStore = Reflux.createStore({
    listenables: actions,
    onFireBall: function(){
        // whoooosh!
    }
});

I think it might crash now, but I haven't reviewed it completely as of right now. Please do check this scenario.

Member

spoike commented Sep 10, 2014

Looks like a nice idea. 👍

Though it would probably make more sense if the methods are prefixed with 'on' and capitalize first letter. E.g.

var actions = Reflux.createActions(["fireBall","magicMissile"]);

var Store = Reflux.createStore({
    listenables: actions,
    onFireBall: function(){
        // whoooosh!
    },
    onMagicMissile: function(){
        // bzzzzapp!
    }
});

As a line comment here is a sample example of how things usually turn out with the action and store relationship:

var actions = Reflux.createActions(["fireBall","magicMissile"]);

// only needs to listen to magicMissile action
var MagicMissileStore = Reflux.createStore({
    listenables: actions,
    onMagicMissile: function(){
        // bzzzzapp!
    }
});

// only needs to listen to fireBall action
var FireBallStore = Reflux.createStore({
    listenables: actions,
    onFireBall: function(){
        // whoooosh!
    }
});

I think it might crash now, but I haven't reviewed it completely as of right now. Please do check this scenario.

@spoike spoike added the enhancement label Sep 10, 2014

@spoike spoike added this to the 0.1.8 milestone Sep 10, 2014

@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 10, 2014

Contributor

Clever! Intuitive, encourages good naming. I like it!

Contributor

krawaller commented Sep 10, 2014

Clever! Intuitive, encourages good naming. I like it!

@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 10, 2014

Contributor

Your dual store scenario now works, and test suite covers missing callbacks.

Contributor

krawaller commented Sep 10, 2014

Your dual store scenario now works, and test suite covers missing callbacks.

@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Sep 10, 2014

Member

Sweet. Will review and add it to 0.1.8 release when I have the time to do so.

Member

spoike commented Sep 10, 2014

Sweet. Will review and add it to 0.1.8 release when I have the time to do so.

@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 10, 2014

Contributor

👍

Was just browsing through my Reflux love song post and realised that these 0.1.8 changes shave quite a few rows off of the already tiny Reflux code examples. Reflux is just so neat, thanx again for this! :)

Contributor

krawaller commented Sep 10, 2014

👍

Was just browsing through my Reflux love song post and realised that these 0.1.8 changes shave quite a few rows off of the already tiny Reflux code examples. Reflux is just so neat, thanx again for this! :)

@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Sep 10, 2014

Member

I'm pondering if it should support stores as listenables as well. The difficulty though is that the stores are currently "unnamed".

Member

spoike commented Sep 10, 2014

I'm pondering if it should support stores as listenables as well. The difficulty though is that the stores are currently "unnamed".

@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 10, 2014

Contributor

Since store.listen and action.listen have the same signatures, doesn't it already support that? Just add it to the object with the name to build the callback name from.

var listenables = {
  coolAction: Reflux.createAction(),
  stuffStore: Reflux.createStore({...})
};

var store = Reflux.createStore({
  listenables:listenables,
  onCoolAction: function(){..},
  onStuffStore: function(){...}
});

Though perhaps having to add it is so messy it's actually easier to just set the listener insinde init.

Contributor

krawaller commented Sep 10, 2014

Since store.listen and action.listen have the same signatures, doesn't it already support that? Just add it to the object with the name to build the callback name from.

var listenables = {
  coolAction: Reflux.createAction(),
  stuffStore: Reflux.createStore({...})
};

var store = Reflux.createStore({
  listenables:listenables,
  onCoolAction: function(){..},
  onStuffStore: function(){...}
});

Though perhaps having to add it is so messy it's actually easier to just set the listener insinde init.

@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Sep 10, 2014

Member

Ah... I stand corrected.

Member

spoike commented Sep 10, 2014

Ah... I stand corrected.

@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 10, 2014

Contributor

Maybe it would be convenient to allow listenables to be an array? Then you could do:

var actions = require("./appactions"), anotherstore = require("./anotherstore");

var store = Reflux.createStore({
  listenables: [actions,{anotherStore:anotherstore}],
  onSomeAction: function(){..},
  onSomeOtherAction: function(){..},
  onAnotherStore: function(){...}
});
Contributor

krawaller commented Sep 10, 2014

Maybe it would be convenient to allow listenables to be an array? Then you could do:

var actions = require("./appactions"), anotherstore = require("./anotherstore");

var store = Reflux.createStore({
  listenables: [actions,{anotherStore:anotherstore}],
  onSomeAction: function(){..},
  onSomeOtherAction: function(){..},
  onAnotherStore: function(){...}
});
@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Sep 10, 2014

Member

Yes, that'd be nice. 👍

Member

spoike commented Sep 10, 2014

Yes, that'd be nice. 👍

@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 10, 2014

Contributor

This process could be made simpler still if we made the listenables API less anal; when registering anAction, we could first look for callback named onAnAction, then anAction if that is missing. That would make the array syntax less of a headache with the key naming.

Contributor

krawaller commented Sep 10, 2014

This process could be made simpler still if we made the listenables API less anal; when registering anAction, we could first look for callback named onAnAction, then anAction if that is missing. That would make the array syntax less of a headache with the key naming.

@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 10, 2014

Contributor

Ok, major refactoring!

  • utils object now has a listenToMany method that takes an object of listenables. Tests added.
  • the listenToMany method adopts the less anal approach of looking first for onActionName, then actionName
  • Stores instances get this method
  • If store definition has listenables prop, that object is sent to listenToMany
  • If the listenables prop is an array, each object is passed to listenToMany
  • ListenerMixin also exposes listenToMany because hey, why not
  • README updated accordingly
Contributor

krawaller commented Sep 10, 2014

Ok, major refactoring!

  • utils object now has a listenToMany method that takes an object of listenables. Tests added.
  • the listenToMany method adopts the less anal approach of looking first for onActionName, then actionName
  • Stores instances get this method
  • If store definition has listenables prop, that object is sent to listenToMany
  • If the listenables prop is an array, each object is passed to listenToMany
  • ListenerMixin also exposes listenToMany because hey, why not
  • README updated accordingly

krawaller added some commits Sep 10, 2014

@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 10, 2014

Contributor

Since it's not really a utility, perhaps it would be cleaner to let listenToMany live in the ListenerMixin module instead? Then createStore can steal it from there instead.

Contributor

krawaller commented Sep 10, 2014

Since it's not really a utility, perhaps it would be cleaner to let listenToMany live in the ListenerMixin module instead? Then createStore can steal it from there instead.

@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 10, 2014

Contributor

Doing that move made me realise; the listenTo method in store contains safety checks that really ought to be in ListenerMixin.listenTo as well. I can't see any need for the two methods to be different, so how about moving store.hasListener to ListenerMixin as well, and simply have createStore mix in ListenerMixin entirely (apart perhaps from the component... methods`)?

Or even cleaner; have listenTo, listenToMany and hasListener in its own module listenerMethods, which is consumed by createStore and ListenerMixin.

Contributor

krawaller commented Sep 10, 2014

Doing that move made me realise; the listenTo method in store contains safety checks that really ought to be in ListenerMixin.listenTo as well. I can't see any need for the two methods to be different, so how about moving store.hasListener to ListenerMixin as well, and simply have createStore mix in ListenerMixin entirely (apart perhaps from the component... methods`)?

Or even cleaner; have listenTo, listenToMany and hasListener in its own module listenerMethods, which is consumed by createStore and ListenerMixin.

@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 10, 2014

Contributor

Doh, wait, I've been too hasty - old createStore.registered stored objects being listened to, while componentWithMixin.subscribed stored the unsubscribers.

If you can hold off until tomorrow I'll take another pass!

Contributor

krawaller commented Sep 10, 2014

Doh, wait, I've been too hasty - old createStore.registered stored objects being listened to, while componentWithMixin.subscribed stored the unsubscribers.

If you can hold off until tomorrow I'll take another pass!

@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Sep 10, 2014

Member

I can hold off the whole week. ;-)

Member

spoike commented Sep 10, 2014

I can hold off the whole week. ;-)

@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 11, 2014

Contributor

This was really fun to reason about! Here's what I ended up with. Previously we had two different use cases when listening to stuff:

  • React components could listen to stuff, and wanted to store the unsub methods for cleanup when they are unmounted
  • Stores could also listen to stuff, but as they commonly have eternal life, they weren't bothered with cleanup. They do however need to check for circular dependencies, so they stored the instances they listened to instead.

But conceptually both the react components and the stores are simply listeners, and should be able to do both things. Perhaps in the future we'll have a listenee mixin so that components too want to check for loops? And maybe a store for whatever reason might need to die? No real reason to divvy things up!

So I ended up making listenTo return a subscription object with two properties:

  • stop: a method that calls unsub returned from .listen, and removes itself from this.subscriptions in the listening object. Unless you pass true to stop, if you do that the array is untouched. Only reason for this is to be able to safely clear all subscriptions in a for loop.
  • listenable: a reference to the instance being listened to.

Thus the subscription object covers both use cases. This only called for a couple of very minor changes in other code:

  • changing listener = this.subscriptions[i] to listener = this.subscriptions[i].listenable in listenerMethods.hasListener
  • changing subscription() to subscription.stop() in subscription removing code.

A minor change but I feel the win is big, as the API is now unified.

I also abstracted out the subscription stopping to listenerMethods, adding stopListening(listener) and stopListeningToAll() there. This means the listenerMixin is now reduced to this sleek beauty:

module.exports = _.extend(Reflux.listenerMethods,{
    componentWillUnmount: Reflux.listenerMethods.stopListeningToAll
});
Contributor

krawaller commented Sep 11, 2014

This was really fun to reason about! Here's what I ended up with. Previously we had two different use cases when listening to stuff:

  • React components could listen to stuff, and wanted to store the unsub methods for cleanup when they are unmounted
  • Stores could also listen to stuff, but as they commonly have eternal life, they weren't bothered with cleanup. They do however need to check for circular dependencies, so they stored the instances they listened to instead.

But conceptually both the react components and the stores are simply listeners, and should be able to do both things. Perhaps in the future we'll have a listenee mixin so that components too want to check for loops? And maybe a store for whatever reason might need to die? No real reason to divvy things up!

So I ended up making listenTo return a subscription object with two properties:

  • stop: a method that calls unsub returned from .listen, and removes itself from this.subscriptions in the listening object. Unless you pass true to stop, if you do that the array is untouched. Only reason for this is to be able to safely clear all subscriptions in a for loop.
  • listenable: a reference to the instance being listened to.

Thus the subscription object covers both use cases. This only called for a couple of very minor changes in other code:

  • changing listener = this.subscriptions[i] to listener = this.subscriptions[i].listenable in listenerMethods.hasListener
  • changing subscription() to subscription.stop() in subscription removing code.

A minor change but I feel the win is big, as the API is now unified.

I also abstracted out the subscription stopping to listenerMethods, adding stopListening(listener) and stopListeningToAll() there. This means the listenerMixin is now reduced to this sleek beauty:

module.exports = _.extend(Reflux.listenerMethods,{
    componentWillUnmount: Reflux.listenerMethods.stopListeningToAll
});

krawaller added some commits Sep 11, 2014

made sure modules don't soil each other
My careless use of _.extend made `listenerMixin` add stuff to
`listenerMethods`. Fixed that bug, and added tests to make sure it
doesn’t happen again.
@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 11, 2014

Contributor

Since this PR has already passed the maximum arrogance border, I also abstracted out all publishing methods from createStore and createAction into a publishingMethods module.

Contributor

krawaller commented Sep 11, 2014

Since this PR has already passed the maximum arrogance border, I also abstracted out all publishing methods from createStore and createAction into a publishingMethods module.

@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 11, 2014

Contributor

Since all listening methods are now in one module, it was easy to move _.handleDefaultData there. Renamed it to fetchDefaultData.

Contributor

krawaller commented Sep 11, 2014

Since all listening methods are now in one module, it was easy to move _.handleDefaultData there. Renamed it to fetchDefaultData.

@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 11, 2014

Contributor

Ok, NOW I'm done. :)

As said, far too many changes for one PR, sorry about that. Give a shout if you need help to make sense of it all!

Contributor

krawaller commented Sep 11, 2014

Ok, NOW I'm done. :)

As said, far too many changes for one PR, sorry about that. Give a shout if you need help to make sense of it all!

@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Sep 11, 2014

Member

Are you really sure that you're done? ;-)

Member

spoike commented Sep 11, 2014

Are you really sure that you're done? ;-)

@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 11, 2014

Contributor

hmmm

Contributor

krawaller commented Sep 11, 2014

hmmm

@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 11, 2014

Contributor

...yes. :)

Contributor

krawaller commented Sep 11, 2014

...yes. :)

@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Sep 11, 2014

Member

Merged into 0.1.8 branch. So I'm closing this for now.

Member

spoike commented Sep 11, 2014

Merged into 0.1.8 branch. So I'm closing this for now.

@spoike spoike closed this Sep 11, 2014

@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Sep 11, 2014

Member

I.e. #67

Member

spoike commented Sep 11, 2014

I.e. #67

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment