React: Warn when reading from event which has been returned to the pool

[kentcdodds]

The title is an assumption based on my experience upgrading. Here's an example of what I'm seeing:

const App = React.createClass({
  getInitialState() {
    return {clicked: 0}
  },
  handleClick(e) {
    console.log('e.target before callback is null?', e.target === null) // e.target is not null here
    this.setState({
      clicked: this.state.clicked + 1
    }, () => {
      console.log('e.target after callback is null?', e.target === null) // e.target is null here
    })
  },
  render() {
    return (
      <div
        onClick={this.handleClick}
      >
        Click me {this.state.clicked}
      </div>
    )
  }
})

ReactDOM.render(<App />, document.getElementById('root'))

Here's a jsbin that demonstrates the behavior with 0.14.6: https://jsbin.com/gasozit/edit?js,console,output

Here's a jsbin with 0.14.7: https://jsbin.com/vaberi/edit?js,console,output

Notice in the console, that the log inside the callback is logging that e.target === null evaluates to true. In my app, I'm passing the event to an onChange handler which utilizes the target. So this update breaks that situation.

This may or may not be a bug. It may just be necessary for avoiding the memory leak. If that's the case, then it's a support request :smile: How should I handle this if I'm passing the event to another callback and I want that event to have references to things like target?

[gaearon]

I may be wrong but I think this is by design (and equivalent to how it worked before a couple of versions back). You can grab the stuff you need from the event object:

  handleClick(e) {
    const { target } = e
    this.setState({
      clicked: this.state.clicked + 1
    }, () => {
      console.log(target)
    })
  },

[kentcdodds]

Thanks for the comment @gaearon. I updated my issue to be more specific about why I'd rather avoid doing that.

In my app, I'm passing the event to an onChange handler which utilizes the target. So this update breaks that situation.

If it were just target, then that'd be simple enough to monkey-patch back onto the event in the callback. But before the callback, the event has the following non-null keys:

["dispatchConfig", "dispatchMarker", "nativeEvent", "type", "target", "currentTarget", "eventPhase", "bubbles", "cancelable", "timeStamp", "defaultPrevented", "isTrusted", "view", "detail", "screenX", "screenY", "clientX", "clientY", "ctrlKey", "shiftKey", "altKey", "metaKey", "getModifierState", "button", "buttons", "pageX", "pageY", "isDefaultPrevented", "isPropagationStopped", "_dispatchListeners", "_dispatchIDs"]

After, only these remain as non-null:

["isDefaultPrevented", "isPropagationStopped"]

So I could definitely keep a reference to those and monkey patch them back on, but I'd love to find another solution :-)

[kentcdodds]

I should point out that the specific code I'm working with is a fork of rackt/react-autocomplete, specifically these lines. So I imagine anyone utilizing any properties from the event in onChange is going to bump into this when they upgrade.

[gaearon]

Why not shallow clone the event if you want to keep it around?

e = { ...e }

[jimfb]

Maybe I'm missing something, but... https://facebook.github.io/react/docs/events.html#event-pooling

Seems like you want to call e.persist(), right?

[kentcdodds]

@gaearon, I guess it just felt hacky and was looking for a "blessed" way to deal with this. I think e.persist() might be it. I'll check it out. Thanks @jimfb!

[gaearon]

I didn’t know persist() existed! This made my day.

[kentcdodds]

That totally worked. I'll PR rackt/react-autocomplete with that. Thanks @jimfb :-)

[gaearon]

Can we warn on accessing target and other properties for the pooled events while they are not used?

[kentcdodds]

Not sure I know what you're suggesting. But maybe this is what you mean. Maybe in development mode we could replace them with getters that will throw a clear error indicating that those properties are removed after the initial handlers have been called and if you need access to them, you should find another way to do what you're trying to do or use persist.

[gaearon]

Maybe in development mode we could replace them with getters that will throw a clear error indicating that those properties are removed after the initial handlers have been called and if you need access to them, you should find another way to do what you're trying to do or use persist.

Yeah, this is what I meant.

[jimfb]

Yeah, probably a good thing to do.

[kentcdodds]

I'd love to make this my first contribution if you don't mind holding off for a bit for me to work on it. :smile: