I have been reviewing this event handler code incorrectly!

Gaurav Gupta
Jan 27 · 3 min read
Photo by Markus Winkler on Unsplash
<button
...
onClick={() => this.props.handleClick()}
/>

Usually when I see code like this in a PR, I suggest the dev to change it to:

<button
...
onClick={this.props.handleClick}
/>

because of unnecessary anonymous function being created in every render, which although isn’t as big a deal, but why not fix it if you easily can?

Though I have been doing this for quite some time now, it was only recently that I realized that my understanding about both these snippets being functionally similar, is a bit flawed, though it works as expected in most of the cases. (Technically, the understanding isn’t flawed, how I perceive the code to be working, has become a bit flawed, by force of habit)

Let’s try to establish the correct understanding with an example:

<button onClick={() => this.props.handleClick()}/>

and somewhere in the parent

handleClick = () => {
console.log('clicked');
}

In this case, both onClick={() => this.props.handleClick()}and onClick={this.props.handleClick}are functionally similar.

But what if the parent function was written like this?

handleClick = (optionalParam = 0) => {
console.log('param value is', optionalParam);
}

Would both the syntax still be functionally similar?
Would you expect 0 to be printed if you use onClick={this.props.handleClick} ?

NO!

any DOM event handler, by default, passes the event as a parameter, and hence in this case the event would be passed as the optionalParam.
This would have worked fine if we use onClick={() => this.props.handleClick()}

This is not specific to React, but with React and other modern frameworks, we tend to use the event object a lot less, as most of the things are handled with state and props, we are more liable to fall into this situation.

Now, I have known this all along, but doing it a particular way over the years, it has become second nature, so now when reviewing code I don’t actually need to see how the handleClick function is defined in the parent, all I observe is that this function is being called without a param, and hence it wouldn’t be expecting an argument, and hence it could be reduced to the ‘better’ syntax.

That said, it is slightly easier to point out the mistake when the handler is defined in the same file, or if the handler explicitly expects a param (in which case, you could expect it to be used with the anonymous function syntax, passing a param). It becomes a bit more easier to ignore if the handler is being passed down multiple levels and specially if the handler expects an optional param, in which case, since you can still call the function without any param, it is not unlikely for the reviewer to think that the ‘better’ syntax should be used here, instead of unnecessarily creating a new function in every render.

This could be worse, if the dev thought that both syntax are functionally similar, and somehow missed testing it in a particular usage where no param is being passed (if the same handler is being called from multiple places), OR, if someone refactored the component later and thought both syntax are functionally similar.

We haven’t faced this issue enough times to decide some general guidelines around how we bind event handlers. So the only takeaway for now is to be more observant with event handlers in PR reviews.

CodeX

Everything connected with Tech & Code