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 ListenerMixinFactory #59

Closed
wants to merge 21 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@krawaller
Contributor

krawaller commented Sep 6, 2014

This pull request adds a ListenerMixinFactory shorthand, which creates the componentDidMount call for you. Instead of doing this:

var myComponent = React.createClass({
  mixins: [Reflux.ListenerMixin],
  componentDidMount: function() {
    this.listenTo(myStore, this.someMethod);
  },
  // rest redacted
});

...you can now do this:

var myComponent = React.createClass({
  mixins: [Reflux.ListenerMixinFactory(myStore,"someMethod"],
  // rest redacted
});

Using this.someMethod as the second argument won't work, as this doesn't point to the instance at this point in time. You can however of course send a callback by reference:

var myCallback = function(){
  // clever stuff goes here
};

var myComponent = React.createClass({
  mixins: [Reflux.ListenerMixinFactory(myStore,myCallback],
  // rest redacted
});

So really just a small convenience, but it removes some boilerplatey stuff. And I thought it was rather cute. :)

krawaller added some commits Sep 6, 2014

Adding the ListenerMixinFactory
Using the ListenerMixinFactory the `componentDidMount` is automatically
created.
@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Sep 6, 2014

Member

Looks clever. Though this won't currently support the use case of a component listening to two or more actions/stores (since adding another ListenerMixinFactory would add another ListenerMixin) which I've seen is a quite common occurrence.

If it could support more than one callbacks (and a blurb about it on the README) and I'd be happy to accept the pull request.

So something along these lines would be nice to have, if you have time to implement this:

var myComponent = React.createClass({
    mixins: [Reflux.ListenerMixinFactory({
        'onActionUpdate': myAction,
        'onStoreUpdate': myStore,
        'onJoinedUpdate': myJoined
    })],
    // rest redacted
});
Member

spoike commented Sep 6, 2014

Looks clever. Though this won't currently support the use case of a component listening to two or more actions/stores (since adding another ListenerMixinFactory would add another ListenerMixin) which I've seen is a quite common occurrence.

If it could support more than one callbacks (and a blurb about it on the README) and I'd be happy to accept the pull request.

So something along these lines would be nice to have, if you have time to implement this:

var myComponent = React.createClass({
    mixins: [Reflux.ListenerMixinFactory({
        'onActionUpdate': myAction,
        'onStoreUpdate': myStore,
        'onJoinedUpdate': myJoined
    })],
    // rest redacted
});
@spoike

View changes

Show outdated Hide outdated src/ListenerMixinFactory.js
@spoike

View changes

Show outdated Hide outdated src/ListenerMixinFactory.js
@spoike

View changes

Show outdated Hide outdated src/ListenerMixinFactory.js
@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 6, 2014

Contributor

Doh, yup, feel silly for not having considered adding multiple listeners.

Your proposed syntax, using method names as keys and listenables as vals, prevent sending non-instance callbacks (function by reference), which seems a bit limiting.

Perhaps we could also support calling the factory with [[listenable,callback],[listenable,callback],...] ? Where callback can be string or function.

Contributor

krawaller commented Sep 6, 2014

Doh, yup, feel silly for not having considered adding multiple listeners.

Your proposed syntax, using method names as keys and listenables as vals, prevent sending non-instance callbacks (function by reference), which seems a bit limiting.

Perhaps we could also support calling the factory with [[listenable,callback],[listenable,callback],...] ? Where callback can be string or function.

@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Sep 6, 2014

Member

Yeah, you're right. I've though I entertained myself with the idea to use non-instance callbacks, but in all cases I leaned over to use instance callbacks anyway. But I can see it used for some quick hacks or spike solutions.

If we're going with the array solution, at least use the arguments array-like internally so the usage looks like this:

var myComponent = React.createClass({
    mixins: [Reflux.ListenerMixinFactory(
        [store1, 'onCallback'],
        [store2, function(arg) { console.log(arg); }]
    )],
    // rest redacted
});

I usually don't like to use arrays as data structure, but with this order it looks almost like how listenTo is used.

Member

spoike commented Sep 6, 2014

Yeah, you're right. I've though I entertained myself with the idea to use non-instance callbacks, but in all cases I leaned over to use instance callbacks anyway. But I can see it used for some quick hacks or spike solutions.

If we're going with the array solution, at least use the arguments array-like internally so the usage looks like this:

var myComponent = React.createClass({
    mixins: [Reflux.ListenerMixinFactory(
        [store1, 'onCallback'],
        [store2, function(arg) { console.log(arg); }]
    )],
    // rest redacted
});

I usually don't like to use arrays as data structure, but with this order it looks almost like how listenTo is used.

@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Sep 6, 2014

Member

Actually we could do it a lot simpler by extending using React's own extender when it goes through it's mixins. I believe React's own mixin extender will chain duplicate callbacks, i.e. componentDidMount for each mixin will be called if such exist.

var myComponent = React.createClass({
    mixins: [
        Reflux.ListenerMixin,
        Reflux.ListenTo(store1, 'onCallback'),
        Reflux.ListenTo(store2, function(arg) { console.log(arg); })
    ]
});

At least this makes more sense than the array solution. What do you think?

Member

spoike commented Sep 6, 2014

Actually we could do it a lot simpler by extending using React's own extender when it goes through it's mixins. I believe React's own mixin extender will chain duplicate callbacks, i.e. componentDidMount for each mixin will be called if such exist.

var myComponent = React.createClass({
    mixins: [
        Reflux.ListenerMixin,
        Reflux.ListenTo(store1, 'onCallback'),
        Reflux.ListenTo(store2, function(arg) { console.log(arg); })
    ]
});

At least this makes more sense than the array solution. What do you think?

@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Sep 6, 2014

Member

Yes, according to the React docs on mixins it does:

A nice feature of mixins is that if a component is using multiple mixins and several mixins define the same lifecycle method (i.e. several mixins want to do some cleanup when the component is destroyed), all of the lifecycle methods are guaranteed to be called.

... and componentDidMount is a "lifecycle method" according to React docs on componentDidMount.

Member

spoike commented Sep 6, 2014

Yes, according to the React docs on mixins it does:

A nice feature of mixins is that if a component is using multiple mixins and several mixins define the same lifecycle method (i.e. several mixins want to do some cleanup when the component is destroyed), all of the lifecycle methods are guaranteed to be called.

... and componentDidMount is a "lifecycle method" according to React docs on componentDidMount.

@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 6, 2014

Contributor

Haha, I just sat down at the computer to share that exact same insight with you! Great minds think alike! :)

The solution is extra elegant when renaming ListenerMixinFactory to ListenTo, in effect turning it to a static method with the exact same name and signature as the instance method. Beautiful!

Yeah, I think this is way prettier than having varying signatures. Well, perhaps we could support your initial syntax suggestion too, just for the hell of it.

I'll fix the PR tomorrow!

Contributor

krawaller commented Sep 6, 2014

Haha, I just sat down at the computer to share that exact same insight with you! Great minds think alike! :)

The solution is extra elegant when renaming ListenerMixinFactory to ListenTo, in effect turning it to a static method with the exact same name and signature as the instance method. Beautiful!

Yeah, I think this is way prettier than having varying signatures. Well, perhaps we could support your initial syntax suggestion too, just for the hell of it.

I'll fix the PR tomorrow!

@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 7, 2014

Contributor

PR updated with renaming and README blurb.

I tried to add tests for cloning an actual React component with multiple ListenTo calls, but faking the life cycle turned out to be a real headache. Tested it manually though and it seems to work just fine.

I also explored doing some fancy stuff inside the provided componentDidMount if we notice that this.listenTo is missing, i.e. we haven't mixed in the ListenerMixin. Goal was to not force the user to provide the mixin manually. This too was a mire, though, and resulted in very brittle code, so I gave that up.

Small PS: I wen't with capital L in ListenTo as in your code example, but seeing it now, perhaps lower case is better? As is it looks like a constructor.

Contributor

krawaller commented Sep 7, 2014

PR updated with renaming and README blurb.

I tried to add tests for cloning an actual React component with multiple ListenTo calls, but faking the life cycle turned out to be a real headache. Tested it manually though and it seems to work just fine.

I also explored doing some fancy stuff inside the provided componentDidMount if we notice that this.listenTo is missing, i.e. we haven't mixed in the ListenerMixin. Goal was to not force the user to provide the mixin manually. This too was a mire, though, and resulted in very brittle code, so I gave that up.

Small PS: I wen't with capital L in ListenTo as in your code example, but seeing it now, perhaps lower case is better? As is it looks like a constructor.

@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 7, 2014

Contributor

Just noticed I completely missed that ListenerMixin.listenTo has a third parameter for an initial callback. Added support & test for that in ListenTo now.

Contributor

krawaller commented Sep 7, 2014

Just noticed I completely missed that ListenerMixin.listenTo has a third parameter for an initial callback. Added support & test for that in ListenTo now.

@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Sep 7, 2014

Member

I tried to add tests for cloning an actual React component with multiple ListenTo calls, but faking the life cycle turned out to be a real headache. Tested it manually though and it seems to work just fine.

I think it is reasonable of us to not test how React runs the mixin's lifecycle methods since it is clearly their concern and not ours. As long as componentDidMount is called after componentWillMount we're safe... and it really shouldn't matter much in which order the ListenTo mixins are called.

I also explored doing some fancy stuff... (snip snip)... This too was a mire, though, and resulted in very brittle code, so I gave that up.

I gave it some thought but as you said, it'd be brittle. However as an alternative we could let each ListenTo just set the unsubscribe function by themselves in a closure. Sort of like this:

module.exports = function (listenable, callback, defaultCallback) {
    var unsubscribe; // enclosed unsub function

    return {
        componentDidMount: function() {
            var actualCallback = this[callback]||callback;
            unsubscribe = listenable.listen(actualCallback, this);
            _.handleDefaultCallback(this, listenable, defaultCallback);
        },
        componentWillUnmount: function() {
            if (unsubscribe) {
                unsubscribe();
            }
        }
    };
};

... and then you can use it without using the ListenerMixin. Does this make sense to you?

Small PS: (snip-snip)

Yes, nice catch @krawaller, you are correct. In terms of code style it should be listenTo since it is a function.

Member

spoike commented Sep 7, 2014

I tried to add tests for cloning an actual React component with multiple ListenTo calls, but faking the life cycle turned out to be a real headache. Tested it manually though and it seems to work just fine.

I think it is reasonable of us to not test how React runs the mixin's lifecycle methods since it is clearly their concern and not ours. As long as componentDidMount is called after componentWillMount we're safe... and it really shouldn't matter much in which order the ListenTo mixins are called.

I also explored doing some fancy stuff... (snip snip)... This too was a mire, though, and resulted in very brittle code, so I gave that up.

I gave it some thought but as you said, it'd be brittle. However as an alternative we could let each ListenTo just set the unsubscribe function by themselves in a closure. Sort of like this:

module.exports = function (listenable, callback, defaultCallback) {
    var unsubscribe; // enclosed unsub function

    return {
        componentDidMount: function() {
            var actualCallback = this[callback]||callback;
            unsubscribe = listenable.listen(actualCallback, this);
            _.handleDefaultCallback(this, listenable, defaultCallback);
        },
        componentWillUnmount: function() {
            if (unsubscribe) {
                unsubscribe();
            }
        }
    };
};

... and then you can use it without using the ListenerMixin. Does this make sense to you?

Small PS: (snip-snip)

Yes, nice catch @krawaller, you are correct. In terms of code style it should be listenTo since it is a function.

@spoike spoike added the enhancement label Sep 7, 2014

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

@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 7, 2014

Contributor

This is getting weird - I sat down now to suggest supplying a closured desubscriber, and what do I find? :)

Will get right on adding that, and lowercasing the l.

Contributor

krawaller commented Sep 7, 2014

This is getting weird - I sat down now to suggest supplying a closured desubscriber, and what do I find? :)

Will get right on adding that, and lowercasing the l.

@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Sep 7, 2014

Member

Great minds think a like...

... or things are starting to get a bit spooky.

Spooky!

Member

spoike commented Sep 7, 2014

Great minds think a like...

... or things are starting to get a bit spooky.

Spooky!

@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 7, 2014

Contributor

Huh, can't figure out what's up with Travis. Suddenly bombs out because it can't find ./listenTo in index.js, which worked fine before and works locally. Cache weirdness, or am I missing something?

Contributor

krawaller commented Sep 7, 2014

Huh, can't figure out what's up with Travis. Suddenly bombs out because it can't find ./listenTo in index.js, which worked fine before and works locally. Cache weirdness, or am I missing something?

@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 7, 2014

Contributor

It was a cache thing - seems Travis is weirded out when a file name change just contains upper->lower or vice versa. Changing the module and the spec file names to something completely different made it work, and still works now that I changed it back.

Contributor

krawaller commented Sep 7, 2014

It was a cache thing - seems Travis is weirded out when a file name change just contains upper->lower or vice versa. Changing the module and the spec file names to something completely different made it work, and still works now that I changed it back.

@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 8, 2014

Contributor

I felt it was iffy that Reflux.listenTo could take string names while the otherwise identical ListenerMixin.listenTo and storeinstance.listenTo could not, so I made them work in the same way too (and added tests).

Although perhaps I should've saved that for another PR, this is getting a bit bloated...

Contributor

krawaller commented Sep 8, 2014

I felt it was iffy that Reflux.listenTo could take string names while the otherwise identical ListenerMixin.listenTo and storeinstance.listenTo could not, so I made them work in the same way too (and added tests).

Although perhaps I should've saved that for another PR, this is getting a bit bloated...

@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Sep 11, 2014

Member

Trying to merge this with #63 and get a lot of merge conflicts. >_>;

Member

spoike commented Sep 11, 2014

Trying to merge this with #63 and get a lot of merge conflicts. >_>;

@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 11, 2014

Contributor

Sorry, I buried my comment about that in all my spam in the other thread - #63 now contains all functionality from here, so if you go with that, this PR can be shot down!

Contributor

krawaller commented Sep 11, 2014

Sorry, I buried my comment about that in all my spam in the other thread - #63 now contains all functionality from here, so if you go with that, this PR can be shot down!

@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Sep 11, 2014

Member

Ok. Closing this PR then.

Member

spoike commented Sep 11, 2014

Ok. Closing this PR then.

@spoike spoike closed this Sep 11, 2014

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