React: Strange second argument passed to event handler (always undefined)

[avchugaev]

I've found a bug.

I've attached event listener with onClick property like this:

<a href="#" onClick={function (event) {
    console.log(arguments.length);   // prints 2
    // arguments[0] = SyntheticEvent - it's ok
    // arguments[1] = undefined - what is this???
}}>Click me</a>

The event listener called with 2 arguments: synthetic event and strange undefined value.

I think, normal behavior is when handler called with a _single_ param - event object.

Windows 10. Chrome 53. React version - 15.2.0.

[avchugaev]

Can please someone confirm is this a bug or it's by design?

[gaearon]

I’m not sure if it was intentional (likely not) but this shouldn’t matter in projects (arguments.length is the only observable difference).

Is there a reason it matters to you?

[syranide]

I'm even getting 3 here, [Proxy, undefined, Event].

[syranide]

Perhaps it's a browser thing, looking at the callstack I don't really see how or why it would be more than 1 argument.

[avchugaev]

Is there a reason it matters to you?

Yes, it is. I've implemented custom basic component:

export class UIComponent extends React.Component {
    // ...

    /**
     *
     * @param {String} fnName
     * @param {*} args
     * @return {Function}
     */
    eventHandler(fnName, ...args) {
        var _this = this;

        return function eventHandler() {
            // Here I expect that arguments length is always 1 - it always get event object.
            _this[fnName](...arguments, ...args);
        };
    }

    // ...

And got a problem with my eventHandler method. This method allows to attach event handlers defined in component classes with some extra data binding, but always with first argument is event object:

class CustomList extends UIComponent {
    render() {
        return (
            <ul>
                {this.props.items.map((item, index) => {
                    return (
                        <li key={index}>
                            <a href="#" onClick={this.eventHandler('onItemClick', index)}>{item.text}</a>
                        </li>
                    );
                })}
            </ul>
        );
    }


    onItemClick(event, index) {
        event.preventDefault();
        console.log(arguments.length);  // 3 - why?
        console.log(index);             // undefined - why?
        console.log(arguments[2]);      // it's index
    }
}

[syranide]

Ok, it's actually React passing it, not sure if it's actually used somewhere. Anyway, I still got a third argument which I can only assume comes from the browser, so it doesn't seem like a safe assumption anyway, unless there's something else I missed.

[avchugaev]

@syranide May I ask you, what version of React do you use?

[avchugaev]

At this point I don't see a reason why React should pass it's internal data to event handler. In my opinion it's a "garbage" data that makes event handlers inconsistent. Or I'm wrong?

[gaearon]

I agree it doesn’t make sense to pass garbage. Can you look at the call stack and figure out how to fix this?

[avchugaev]

After quick look I can't figure out what caused a problem.
Here is calls stack (first 2 calls are my cusstom methods):

onItemClick @   DemoSlides.jsx:120
__uiKitEventHandler @   UIComponent.jsx:68
ReactErrorUtils.invokeGuardedCallback   @   react.js:11091
executeDispatch @   react.js:2893
executeDispatchesInOrder    @   react.js:2916
executeDispatchesAndRelease @   react.js:2348
executeDispatchesAndReleaseTopLevel @   react.js:2359
forEachAccumulated  @   react.js:17714
processEventQueue   @   react.js:2535
runEventQueueInBatch    @   react.js:11116
handleTopLevel  @   react.js:11127
handleTopLevelImpl  @   react.js:11205
perform @   react.js:16969
batchedUpdates  @   react.js:10321
batchedUpdates  @   react.js:14670
dispatchEvent   @   react.js:11282

Also I found (after switched from minified version to regular version) - on regular React build my event handler got 4 params (as @syranide ) said!

[aweary]

I believe the problem is in ReactErrorUtils. It binds additional arugments to boundFunc
https://github.com/facebook/react/blob/v15.3.2/src/renderers/shared/utils/ReactErrorUtils.js#L68

var boundFunc = func.bind(null, a, b);

In this case a is the Proxy and b is undefined which is why you get

[Proxy, undefined, Event]

It looks like this is no longer the case on master. It just binds one argument, a, https://github.com/facebook/react/blob/master/src/renderers/shared/utils/ReactErrorUtils.js#L76

So it only binds Proxy to the event handler now.

[avchugaev]

So what is the purpose of a and b? Why not just push only event object to event handler?

[avchugaev]

@Aweary, is Proxy a SyntheticEvent?

[aweary]

@achugaev93 yes, it seems to be a proxy to the SyntheticEvent.

[aweary]

@achugaev93 you can see in SyntheticEvent.js#L207-L231 that a proxy is used in dev when supported.

[avchugaev]

@Aweary, it's a good news. Waiting for a next release.

React team, you should make a serious refactor of that module. It's look like a mess of code ;) Fix params and variables naming...

[gaearon]

Thanks for advice. Perhaps you could send a PR to help us?

[aweary]

@achugaev93 a PR would be much appreciated! I'd be happy to review.

Keep in mind that ReactErrorUtils is meant to be a general purpose module for safe function invocation, for example we use invokeGaurdedCallback when calling componentWillUnmount. This is why the argument names are generic, since they can represent almost any generic argument.

Though it is kind of confusing that we use dispatchEvent for all cases in DEV even though its used outside the event system. Do you know why we do that @gaearon?

[avchugaev]

Guys. I do not have a complete picture of how the React works from the inside, so in this situation from me will be of little use.

[aweary]

Guys. I do not have a complete picture of how the React works from the inside, so in this situation from me will be of little use.

Totally understandable, the codebase can be dense sometimes. But if you _wanted_ to give it a try I would be happy to work with you on your PR! Otherwise one of us will try and get to it when we can 👍

I'm going to close this issue out because I think we've identified what those additional arguments are? You should expect to see at least SyntheticEvent (or Proxy) and Event (the native event the browser is passing in, once the changes is master are released.

[gaearon]

Though it is kind of confusing that we use dispatchEvent for all cases in DEV even though its used outside the event system. Do you know why we do that @gaearon?

I don’t understand why that abstraction exists. ¯_(ツ)_/¯

[avchugaev]

@Aweary, I don't understand why it should send native event as second argument? Native event instance _already_ attached as nativeEvent property of SyntheticEvent, right?

[aweary]

@achugaev93 it's a side effect of the current abstraction. Since we're actually dispatching a empty event the function is called as an event handler and gets passed the event from that mock event. The event you see there isn't actually the same as SyntheticEvent.nativeEvent, its the event that's just used to invoke the event handler. It should have a type of react-{type}.

This should only be the case in dev though, so it's kind of weird. I'm actually going to reopen this until we can either:

• fully justify why this abstraction exists OR
• remove this abstraction

[gaearon]

ReactErrorUtils.invokeGuardedCallback has four arguments. I can't find any callsites that call it with all four arguments. Let's just remove the b argument and the issue should be resolved.

[aweary]

I believe we can close this now thanks to https://github.com/facebook/react/pull/7610

[smorin]

@stevemorin

[smorin]

@smorin

[smorin]

@aweary or anyone else on this thread.

I am trying to understand what arguments are "officially" supported part of the eventhandler class back specification according to "React". It seems that three arguments are being passed.

1. SyntheticEvent
2. object (think it's the node of the onClick handler
3. undefined (not sure what this third object is supposed to be.

Looking at the documentation, it doesn't mention anything more than the SyntheticEvent. How are you supposed to get access to the node/dom object and what is the "official" object the second argument points to? Is it officially supported?

Also if your only supposed to use SyntheticEvent it is appearing to not contain any reference to the node/DOMelement of the onClick event? (See screen shot below)

The version of React is 15.5.4 according to this

    "react": {
      "version": "15.5.4",
      "resolved": "https://registry.npmjs.org/react/-/react-15.5.4.tgz",
      "integrity": "sha1-+oPrAVBqsjfNwcjDsc6o3gEr8Ec=",
      "dev": true
    },

This is modified code from semantic-ui-react

```import React, { Component } from 'react'
import { Menu, Segment } from 'semantic-ui-react'

export default class MenuExampleSecondaryPointing extends Component {
state = { activeItem: 'home' }

handleItemClick2 = (e, { name } ) => {
console.log(name:${Object.keys(name)},${name})
this.setState({ activeItem: name })
}

handleItemClick3 = () => {
console.log(name:${Object.keys(arguments)})
let a = arguments
for (let index = 0; index < a.length; ++index) {
console.log(a[index]);
console.log(obj - keys:${Object.keys(a[index])})
}
this.setState({ activeItem: name })
}

thefunction() {
console.log(args.len: ${arguments.length})
}

handleItemClick = (e, n, l = {} ) => {
window.thee = e
window.then = n
window.thel = l
this.thefunction()
console.log(e:${Object.keys(e)})
console.log(n:${Object.keys(n)})
console.log(l:${Object.keys(l)})
this.setState({ activeItem: n.name })
}

render() {
const { activeItem } = this.state

return (
  <div>
    <Menu pointing secondary>
      <Menu.Item name='home' active={activeItem === 'home'} onClick={this.handleItemClick} />
      <Menu.Item name='messages' active={activeItem === 'messages'} onClick={this.handleItemClick} />
      <Menu.Item name='friends' active={activeItem === 'friends'} onClick={this.handleItemClick} />
      <Menu.Menu position='right'>
        <Menu.Item name='logout' active={activeItem === 'logout'} onClick={this.handleItemClick} />
      </Menu.Menu>
    </Menu>

    <Segment>
      <img src='/assets/images/wireframe/media-paragraph.png' />
    </Segment>
  </div>
)

}
}
```

@liquidweavergit

[gaearon]

I think the only argument that’s part of the React contract is the event. If you see other arguments there it's a bug. That said if you get these arguments from a third party library, that library could be adding arguments.

[smorin]

Okay thanks, will change the code to make that assumption.

[smorin]

Thanks @gaearon