React: Loosen up type requirements for event handlers

[jeffijoe]

Do you want to request a feature or report a bug?

Feature

What is the current behavior?

When adding event handlers, it is common practice to do something like:

const MyButton = ({ canClick, onClick }) =>
  <div onClick={canClick && onClick}></div>

This was fine in React 15.x, but in 16 it reports a warning, which is technically correct:

Expected onClick listener to be a function, instead got a value of boolean type.

However, this now forces you to use the more verbose variant:

const MyButton = ({ canClick, onClick }) =>
  <div onClick={(canClick && onClick) ? onClick : undefined}></div>

What is the expected behavior?

I think it makes sense to allow null, false, and undefined in addition to function types for event handlers. Or just anything "falsy", although that may be too much to ask.

I definitely understand the rationale from a type safety perspective, but this does make it less pragmatic. I am personally a huge fan of how JS evaluates null, 0, "" and undefined to false, and it reduces the amount of boilerplate needed to conditionally wire up handlers.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

React 16, all browsers. This did not emit a warning in React 15 and below.

[gaearon]

It seems like this would work just fine, would it not?

const MyButton = ({ canClick, onClick }) =>
  <div onClick={canClick ? onClick : null}></div>

This doesn’t look verbose to me, and is very explicit. Of course, if the caller passes false as onClick, you’d see a warning, but presumably you already declare onClick as a func proptype (or a function type in Flow/TypeScript) so that would warn anyway.

[jeffijoe]

but presumably you already declare onClick as a func proptype

I don't always use prop types, but when I do, I allow func, null, false to avoid this warning.

(canClick && onClick) ? onClick : undefined

Doing this gets around the parent passing in falsy values.

[gaearon]

Right. I understand it can be an annoying change. But surely, doing something like onClick={canClick ? onClick : null} at every level where necessary (which probably isn’t very often) is not much more verbose than doing onClick={canClick && onClick}?

[jeffijoe]

You are certainly right that {canClick ? onClick : null} is less verbose than (canClick && onClick) ? onClick : undefined, but I am explicitly trying to remove the responsibility of checking for falsy values from the consumer.

This issue is only related to event handlers on native DOM elements — what custom components choose to enforce (or don't enforce) is out of the scope of this issue. 😄

For example, when you declare that your component explicitly accepts an onClick handler, you have to write a prop type validation anyway (if for example you are following a style guide which a lot of people are).

My goal with this issue is to reduce the amount of forced type-checking imposed on React consumers (not everyone likes their code typed 😉) in order to be more pragmatic. I think type systems in JS should be opt-in which it has been with React up until v15.

[gaearon]

If there is a lot of support for this we can certainly change it back.

I’m just not sure that the use case of conditional event handlers is common enough that a ternary is worse than a boolean shortcut. And in my experience, most components in the ecosystem use either PropTypes or Flow/TS. So, again, they already push that responsibility to the consumer anyway.

Would you want to special case booleans here? What should the behavior be if true is passed? What about 0? Or empty string? What about other numbers or strings?

Should event handlers be treated differently from attributes? We warn on unexpected booleans for attributes because there’s no way to understand what the user meant if they pass a boolean for something that doesn’t take a boolean. They might not even realize an attribute doesn’t take a boolean (such as the case with autoComplete which takes 'on' and 'off', 'name', etc). Would it be easy to remember that attributes and events have different tolerance to type requirements?

[jeffijoe]

Should event handlers be treated differently from attributes? We warn on unexpected booleans for attributes because there’s no way to understand what the user meant if they pass a boolean for something that doesn’t take a boolean. They might not even realize an attribute doesn’t take a boolean (such as the case with autoComplete which takes 'on' and 'off', 'name', etc). Would it be easy to remember that attributes and events have different tolerance to type requirements?

I was actually going to go into detail about that but didn't want to bother in case the proposal was shut down immediately — thank you for being so open-minded!

If true is passed, that should be an error, but I honestly doubt that would ever happen — the idea is to use the boolean shortcut approach to either avoid adding a handler, or add the result of the truthy expression as a handler — but of course the truthy value _must_ be a function.

I think 0 and "" (being falsy values) should be treated like false, undefined and null, so those should not warn either.

If you use autoComplete={false}, that is an error because you obviously intended to disable autoComplete, so in that regard the current behavior is perfect!

So yes, I guess that means event handlers should be treated differently.

But I agree, let's wait and see what others think — heck, maybe I'm the only one who thinks this is annoying.

P.S: unrelated to this particular issue but related to typing in React: in a render method, sometimes to avoid rendering an empty block, the following shorthand is often used:

const commentCount: number = this.props.commentCount
return (
  <div>
    {commentCount &&
      <div>{commentCount} comments</div>
    }
  </div>
)

If commentCount was boolean or a string, you would get the expected output when falsy — that is, the "comments" block is not rendered. But when commentCount is a numerical 0, a text node with "0" is rendered because numbers are valid elements.

This is by no means a problem, I am only pointing this out to illustrate how there are other "gotchas" related to typing that React prop types do not catch — and it shouldn't, because it's perfectly valid. But it sure is a headscratcher when you first see it, because not everyone knows that the false-branch value is the result of a logical && binary expression when it evaluates to false. 😉

[gaearon]

Let’s wait to hear more people about this.

[sanniassin]

I'd like to have this feature too, but it's hard to add anything to what @jeffijoe said. It's all about taste, whether you like pragmatic "hacks" or not.

[Strate]

Just fall into this issue after upgrade to react 16. It is very common for me to do onClick={this.props.disabled || this.props.onClick}, or similar things. Also it is match with exist behaviour of jsx: <div>{this.props.visible && <span />}</div>. I'd like to have this feature too.

[gaearon]

It seems like practically there’s not enough demand for this. So we’ll leave it as is.

[mishra1947]

same issue is coming with onMouseDown event.
Warning: Expected onMouseDown listener to be a function, instead got a value of boolean type.
onMouseDown={(e)=>this.setSwipedIndex(e, index)}