Requiring event listeners to be functions for binding

[toddself]

Fixes #354.

This ensures that passed in listeners are functions so that they can be called correctly.

Throws if not. (matches the pattern Ember, Backbone and (I believe) Angular set forth).

Working on the CLA process via e-mail with Pete Hunt (pete@instagram.com) as mentioned in the #reactjs chat.

[zpao]

Great catch (and I love that you wrote tests)!

We have a function called invariant, so just require that at the top var invariant = require('invariant'); and then use it here. It'll throw errors for you.

Then let's just use typeof here and put it first thing in the function.
invariant(typeof listener === 'function', 'error message here %s', registrationName);

[toddself]

oh hey i just saw this. let me make these changes

[sophiebits]

I think this change is a little weird as you may want to bind a handler conditionally on some flag, so it's nice if null, etc. act as no handler:

<button onClick={!disabled && this.handleClick} />

What do you think?

[andreypopp]

@spicyj agreed, I use this quite often

[petehunt]

What if we checked for truthiness first? I know this doesn't fix the undefined use case, but it fixes an error if you try to add a random object as an event listener, right?

[sophiebits]

Yeah, we do that in ReactUpdates:

  invariant(
    !callback || typeof callback === "function",
    'enqueueUpdate(...): You called `setProps`, `replaceProps`, ' +
    '`setState`, `replaceState`, or `forceUpdate` with a callback that ' +
    'isn\'t callable.'
  );

[toddself]

@zpao Yes -- sorry I misse dthat.

@spicyj & @andreypopp -- there are several ways to rectify that.

1. Could you do:

!disabled ? onClick={this.handleClick} : false

1. Or you could generate a noop method:

<button onClick={disabled ? this.noop : this.handleClick} />

1. Or, if depending on how you're delegating events you could handle the noop case in your event handler:

handleClick: function(event){
   if(this.disabled) return

I feel like giving appropriate feedback rather than failing silently is always a preferable case, especially when other UI-event-binding libraries all follow the throw an error if the handler isn't a function (either at bind-time or run-time).

[toddself]

@petehunt checking for truthiness first would go along way, but it would be really nice at run-time if a handler was bound to an event and it didn't exist to provide an appropriate error to the user.

Do you think that might be more appropriate? Let you do what you want at bind-time, but when it comes to fire the event if the handler isn't a function it throws an error?

[zpao]

Nit for when we figure out exactly what to do: just put the string in here directly. We strip it out in our minification step. You can see how we handle the long line issue (https://github.com/facebook/react/blob/master/src/addons/transitions/ReactTransitionGroup.js#L41-L45 for example)

[toddself]

@zpao we don't accept pull requests internally that don't have test cases, so it's second hand nature now 😄 But I appreciate the enthusiasm!

[petehunt]

What's this doing here? Do we need it? It's not really written to our conventions (specifically relative paths, requiring at the end of a file etc)

[toddself]

So odd. I don't remember putting that line there at all...

This has been resolved and history cleaned.

[petehunt]

CLA is processed, thanks!

[toddself]

@petehunt thanks for the help with the CLA!

[zpao]

I think we're just going to let this one live as is. We have a lot of invariants now and I think we'd like to move towards having a distinction between warn and throw invariants. cc @petehunt