added static versions of bindAsArray and bindAsObject

[krawaller]

This PR employs a mixin factory pattern to make a static version of bindAsArray and bindAsObject.

Instead of doing this...

var ExampleComponent = React.createClass({
  mixins: [ReactFireMixin],
  componentWillMount: function(){
    this.bindAsArray(firebaseRef,"propname",this.cancel);
  }
});

...you can simply do this:

var ExampleComponent = React.createClass({
  mixins: [ReactFireMixin.bindAsArray(firebaseRef,"propname","cancel")],
});

The static versions will then set up the componentWillMount method for you automatically.

To harmonise with not being able to supply this.cancel in the static method (since this doesn't point to the instance at that point in time) I made all versions also work with a string as a cancel argument, which is then assumed to be the name of a method on the instance.

[coveralls]

Coverage increased (+0.3%) when pulling 9b03471 on krawaller:master into cad4742 on firebase:master.

[jwngr]

What exactly is this check ensuring? It seems a bit of a hacky solution. Are there examples of using static methods like this in the React docs that I can take a look at?

[krawaller]

This check tests how the method is being used. If it is statically like in my new syntax:

var ExampleComponent = React.createClass({
  mixins: [ReactFireMixin.bindAsArray(firebaseRef,"propname","cancel")],
});

...then this doesn't point to a component instance (it doesn't have a setState method), and what we want is to return a mixin object with a componentWillMount method that does the _bind call.

If this has a setState method then it is a component instance, which means the firebase method is being called in the usual way:

var ExampleComponent = React.createClass({
  mixins: [ReactFireMixin],
  componentWillMount: function(){
    this.bindAsArray(firebaseRef,"propname",this.cancel);
  }
});

I agree that it tastes a bit hacky, but we need to test somehow whether or not we're currently being used as a static method or an instance method.

We could of course also define these as two compeltely different methods, but I felt this way was easier since it means the fewest changes to the existing codebase.

I don't know of any examples in React that employs the same approach (I call it "mixin factories", dunno if there's a more academically correct name).

[jwngr]

Thanks for the PR! This seems like a solid change overall. I do have a question about the overall usefulness of the change though. Is providing the static versions of these methods just a convenience feature? Is this standard practice for other React mixins?

[krawaller]

Yes, just a convenience! No, not standard practice, but partly because it mostly isn't applicable. Many mixins have lots of methods that I might want to call several times over during the life cycle of a component, and in those cases having static versions doesn't really make sense.

But in the case of reactfire I only want to make one or a few calls as the component is created, which makes it a prime target for this syntax. Same as I did with Reflux.

The general usefulness can definitely be debated. But since the API is so minimal anyway, and the static version has the same signature, my feeling is that the cognitive cost of the addition is very low.

[pwmckenna]

I'd use it :)

[jwngr]

So I know it's been a long time since I've commented here, but I am going to close this PR. I totally understand where you are coming from but I don't think this method really adds a lot of value to the mixin. I also have not really seen this type of thing done with other mixins, so I'm wary to implement it here. A part of me also thinks this is a bit superfluous as it is already really easy to do this. Finally, if you ever wanted to add a cancel callback to your bindAsArray() method (which you should be doing in a production app), then adding it at this place seems unwieldy. That logic should live elsewhere in your code.

I do appreciate your contribution and the discussion. Hopefully ReactFire is working well for you!