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

bind methods in store #74

Closed
krawaller opened this Issue Sep 14, 2014 · 11 comments

Comments

Projects
None yet
3 participants
@krawaller
Contributor

krawaller commented Sep 14, 2014

Methods in React components are automatically bound to the component instance, saving you from having to do any this.onClick = _.bind(this.onClick,this) stuff.

This is convenient and something you quickly get used to, so it feels like an annoyance not to have this convenience in Reflux stores. This crops up pretty often, as stores frequently need to use methods as callbacks for various databases, REST API:s etc.

So, question; should store methods be autobound to the instance?

@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Sep 14, 2014

Member

They are supposed to be bound to the store instance in the listenTo function. Have we regressed in 0.1.8 branch?

Member

spoike commented Sep 14, 2014

They are supposed to be bound to the store instance in the listenTo function. Have we regressed in 0.1.8 branch?

@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 14, 2014

Contributor

It works when you use the Reflux listening/listenable API, but what I was getting at is that the store often sets callback on non-Reflux stuff, as it is the contact point between your application and outside data sources.

For instance, here is the (redacted) code for one of my stores that listens to a Firebase stream:

var Reflux = require('../lib/reflux'),
    Firebase = require("firebase"),
    chatRef = new Firebase("https://riaht2014.firebaseio.com/web/data/chat");

module.exports = Reflux.createStore({
  init: function(){
    chatRef.on("value",this.updateChat.bind(this));
  },
  updateChat: function(snapshot){
    this.trigger((this.last = snapshot.val()||{}));
  }
});

I set updateChat as a callback to catch the Firebase data, but have to bind it to the instance context for the contained this.trigger call to work. It's a very small thing, of course, but it feels alien as you don't have to do it when in React or Reflux land.

Btw, React + Reflux + Firebase = ❤️ ❤️ ❤️

Contributor

krawaller commented Sep 14, 2014

It works when you use the Reflux listening/listenable API, but what I was getting at is that the store often sets callback on non-Reflux stuff, as it is the contact point between your application and outside data sources.

For instance, here is the (redacted) code for one of my stores that listens to a Firebase stream:

var Reflux = require('../lib/reflux'),
    Firebase = require("firebase"),
    chatRef = new Firebase("https://riaht2014.firebaseio.com/web/data/chat");

module.exports = Reflux.createStore({
  init: function(){
    chatRef.on("value",this.updateChat.bind(this));
  },
  updateChat: function(snapshot){
    this.trigger((this.last = snapshot.val()||{}));
  }
});

I set updateChat as a callback to catch the Firebase data, but have to bind it to the instance context for the contained this.trigger call to work. It's a very small thing, of course, but it feels alien as you don't have to do it when in React or Reflux land.

Btw, React + Reflux + Firebase = ❤️ ❤️ ❤️

@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Sep 14, 2014

Member

The thing is that I'd like to support anonymous functions in listenTo functions, in order to have some private functions. A crude example:

this.listenTo(action, function() {
    this.trigger('dude');
});

So to support both cases, I can't think of a method other than bind the methods twice, which is a bit wasteful.

It'd be better if there was a way to do this with firebase stream:

module.exports = Reflux.createStore({
    init: function() {
        this.listenTo(chatRef, this.updateChat);
    },
    updateChat: function(snapshot) { /* ... */ }
});

I did some implementation for BaconJS #52, but I realize now that we probably need to do another approach. I believe it's better to support other kinds of streams by letting the user extend the listenerMethods.

Thankfully I think it's possible to do this in 0.1.8:

// Before any stores are created
Reflux.listenerMethods.listenToFirebase = function(ref, func) {
    var self = this,
        args = arguments;
    ref.on("value", function() {
        func.apply(self, args);
    });
};

// Usage (in store and component):
this.listenToFirebase(chatRef, this.updateChat);
Member

spoike commented Sep 14, 2014

The thing is that I'd like to support anonymous functions in listenTo functions, in order to have some private functions. A crude example:

this.listenTo(action, function() {
    this.trigger('dude');
});

So to support both cases, I can't think of a method other than bind the methods twice, which is a bit wasteful.

It'd be better if there was a way to do this with firebase stream:

module.exports = Reflux.createStore({
    init: function() {
        this.listenTo(chatRef, this.updateChat);
    },
    updateChat: function(snapshot) { /* ... */ }
});

I did some implementation for BaconJS #52, but I realize now that we probably need to do another approach. I believe it's better to support other kinds of streams by letting the user extend the listenerMethods.

Thankfully I think it's possible to do this in 0.1.8:

// Before any stores are created
Reflux.listenerMethods.listenToFirebase = function(ref, func) {
    var self = this,
        args = arguments;
    ref.on("value", function() {
        func.apply(self, args);
    });
};

// Usage (in store and component):
this.listenToFirebase(chatRef, this.updateChat);
@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Sep 14, 2014

Member

I think we can do it this way instead. Let the user decide if they want it or not. I.e. create seperate modules for RxJS, BaconJS and Firebase.

Member

spoike commented Sep 14, 2014

I think we can do it this way instead. Let the user decide if they want it or not. I.e. create seperate modules for RxJS, BaconJS and Firebase.

@krawaller

This comment has been minimized.

Show comment
Hide comment
@krawaller

krawaller Sep 16, 2014

Contributor

Nod, true, having the listener methods abstracted out into their own module opens up some neat opportunities for service-specific augmentations.

Again you're right! :)

Contributor

krawaller commented Sep 16, 2014

Nod, true, having the listener methods abstracted out into their own module opens up some neat opportunities for service-specific augmentations.

Again you're right! :)

@rymohr

This comment has been minimized.

Show comment
Hide comment
@rymohr

rymohr Oct 18, 2014

Contributor

With react, all component methods are bound regardless of where they're used. Binding anonymous functions is your responsibility.

Autobinding the callbacks given to listenTo is more of a backbone behavior. I like it, but I don't think it should trump react's patterns. Why can't we have both?

A few double binds are worth it just to follow the principle of least surprise and make it easier to pass store callbacks around without worrying about context. Much more inline with react's philosophy of designing so users "fall into the pit of success".

Contributor

rymohr commented Oct 18, 2014

With react, all component methods are bound regardless of where they're used. Binding anonymous functions is your responsibility.

Autobinding the callbacks given to listenTo is more of a backbone behavior. I like it, but I don't think it should trump react's patterns. Why can't we have both?

A few double binds are worth it just to follow the principle of least surprise and make it easier to pass store callbacks around without worrying about context. Much more inline with react's philosophy of designing so users "fall into the pit of success".

@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Oct 20, 2014

Member

@rymohr do you have any usecases for passing around store callbacks?

In @krawaller's example, it was to listen to another type of "listenable source". As mentioned above, I believe it's better to just amend the mixins that Reflux uses, so it can be reused easily throughout your application. Admittedly the mixins are not well documented in the README docs and it should probably be documented somewhere (though the internals of reflux have been in a state of flux... heh).

I agree with the notion that a library should have you "fall into the pit of success" but if this is not really a big problem for users of Reflux then it really isn't that big of a pit. I might change my mind on this... but I currently never ran into this problem in my own projects. I can admit that I'm biased though so any insight in how others use it would be very helpful.

Member

spoike commented Oct 20, 2014

@rymohr do you have any usecases for passing around store callbacks?

In @krawaller's example, it was to listen to another type of "listenable source". As mentioned above, I believe it's better to just amend the mixins that Reflux uses, so it can be reused easily throughout your application. Admittedly the mixins are not well documented in the README docs and it should probably be documented somewhere (though the internals of reflux have been in a state of flux... heh).

I agree with the notion that a library should have you "fall into the pit of success" but if this is not really a big problem for users of Reflux then it really isn't that big of a pit. I might change my mind on this... but I currently never ran into this problem in my own projects. I can admit that I'm biased though so any insight in how others use it would be very helpful.

@rymohr

This comment has been minimized.

Show comment
Hide comment
@rymohr

rymohr Oct 20, 2014

Contributor

One case is wanting to map|reduce|filter|etc over arrays using a store method. You also run into it with async calls within stores. It's easy to work around both but it catches you off guard coming from react. Especially since the store constructor and the component constructors follow such a similar syntax.

It's useful, it's an established react pattern, and it's fast (a decent computer can bind a million functions per second)... choosing not to autobind store methods for waste reasons just doesn't make sense to me.

Contributor

rymohr commented Oct 20, 2014

One case is wanting to map|reduce|filter|etc over arrays using a store method. You also run into it with async calls within stores. It's easy to work around both but it catches you off guard coming from react. Especially since the store constructor and the component constructors follow such a similar syntax.

It's useful, it's an established react pattern, and it's fast (a decent computer can bind a million functions per second)... choosing not to autobind store methods for waste reasons just doesn't make sense to me.

@spoike spoike added the enhancement label Oct 21, 2014

@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Oct 21, 2014

Member

Ok @rymohr you've convinced me. I'd be happy to accept a pull request on auto binding if it doesn't add much maintenance problems and works in browsers without native Function.prototype.bind.

Member

spoike commented Oct 21, 2014

Ok @rymohr you've convinced me. I'd be happy to accept a pull request on auto binding if it doesn't add much maintenance problems and works in browsers without native Function.prototype.bind.

@spoike spoike reopened this Oct 21, 2014

@rymohr

This comment has been minimized.

Show comment
Hide comment
@rymohr

rymohr Oct 21, 2014

Contributor

Thanks @spoike, see #100

Contributor

rymohr commented Oct 21, 2014

Thanks @spoike, see #100

@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Dec 7, 2014

Member

Fixed in #100

Member

spoike commented Dec 7, 2014

Fixed in #100

@spoike spoike closed this Dec 7, 2014

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