Undocumented alteration of `onClick` arguments

[wikiwong]

After upgrading from 15.3.2 to React/ReactDOM 15.4.0 , I noticed that the onClick synthetic event (and possibly others) is receiving arguments differently.

In 15.3.2, the onClick event (without any custom arguments being passed) receives 3 arguments

0 - Proxy
1 - undefined
2 - Event
(observed in the console of this jsfiddle: https://jsfiddle.net/u7b6zhfm/1/)

In 15.4.0, the onClick arguments look like:
0 - Proxy
1 - Event
(Again viewable in the console here: https://jsfiddle.net/ujuzLhse/)

This ended up causing problems in my application because in 15.3.2, I was passing an additional argument to a click handler, and expecting it to be received as the second argument. Once I upgraded to 15.4.0, the argument was being received as the third (after Proxy and Event).

I'm not so sure this is to be considered an "issue" with React, but I didn't see anything in the release notes in the way of event handling, so thought I would raise it just in case.

The way I ended up adjusting for this is by using the spread/rest operator when passing/receiving arguments in the event handler function, which may be the approach I should have taken to begin with

PS: Thank you for all the hard work you guys put into this, super excited for fiber! Rock on 🎸

[aweary]

Thanks for the issue @wikiwong! You can read about what that those additional arguments are/where in this issue: #7902

Basically, an additional argument that ended up being undefined was being bound to the event handler, and that was fixed. You should really just ignore any argument coming after Proxy (which is the SyntheticEvent).

This ended up causing problems in my application because in 15.3.2, I was passing an additional argument to a click handler, and expecting it to be received as the second argument. Once I upgraded to 15.4.0, the argument was being received as the third (after Proxy and Event).

The way I pass additional arguments is to bind them in render when passing in the event handler

<div onClick={this.handleClick.bind(this, id)} />

That way the arguments should come before the SyntheticEvent

handleClick(id, event) {
 // ...
}

Doing it this way you shouldn't be affected by the order or number of arguments that are passed to the event handler, as long as the SyntheticEvent is still passed first by React.

Since this is essentially a bug fix I'm going to close this out, but thanks for taking the time to fill it out!

[gaearon]

Why is there even a second argument?

[aweary]

@gaearon because we call the bound event handler by dispatching a custom event here using a fake node. So that second argument ends up being the fake event.

[gaearon]

Can we make a closure there that omits that argument? Then number of arguments should match in dev and prod.

[aweary]

Can we make a closure there that omits that argument? Then number of arguments should match in dev and prod.

It would mean an extra function call for every event dispatch in dev but yeah, we can do it

[gaearon]

Shouldn't be costly as browser events don't come at enormous speeds. Wanna make a PR?

[aweary]

PR opened at #8363

[wikiwong]

Thanks @gaearon and @aweary !

[wikiwong]

@aweary fyi I went down the route of binding the event handler in render, but my linter yelled at me. The reasoning I found pretty interesting: https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-no-bind.md

[smorin]

@smorin