New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn when reading from event which has been returned to the pool #5939

Closed
kentcdodds opened this Issue Jan 29, 2016 · 13 comments

Comments

Projects
None yet
3 participants
@kentcdodds
Contributor

kentcdodds commented Jan 29, 2016

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 😄 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

This comment has been minimized.

Member

gaearon commented Jan 29, 2016

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

This comment has been minimized.

Contributor

kentcdodds commented Jan 29, 2016

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

This comment has been minimized.

Contributor

kentcdodds commented Jan 29, 2016

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

This comment has been minimized.

Member

gaearon commented Jan 29, 2016

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

e = { ...e }
@jimfb

This comment has been minimized.

Contributor

jimfb commented Jan 29, 2016

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

This comment has been minimized.

Contributor

kentcdodds commented Jan 29, 2016

@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

This comment has been minimized.

Member

gaearon commented Jan 29, 2016

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

@kentcdodds

This comment has been minimized.

Contributor

kentcdodds commented Jan 29, 2016

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

@kentcdodds kentcdodds closed this Jan 29, 2016

@gaearon

This comment has been minimized.

Member

gaearon commented Jan 29, 2016

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

kentcdodds added a commit to kentcdodds/react-autocomplete that referenced this issue Jan 29, 2016

[fixed] handleChange should use event.persist()
As noted in [this issue on React](facebook/react#5939), `0.14.7` (released yesterday) introduces some memory leak fixes that result in `event` having most of its properties (like `target` for example) set to `null`.

Calling `event.persist()` will prevent React from removing those properties ([docs](https://facebook.github.io/react/docs/events.html#event-pooling)).
@kentcdodds

This comment has been minimized.

Contributor

kentcdodds commented Jan 29, 2016

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

This comment has been minimized.

Member

gaearon commented Jan 29, 2016

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

This comment has been minimized.

Contributor

jimfb commented Jan 29, 2016

Yeah, probably a good thing to do.

@jimfb jimfb reopened this Jan 29, 2016

@kentcdodds

This comment has been minimized.

Contributor

kentcdodds commented Jan 29, 2016

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. 😄

@jimfb jimfb changed the title from React 0.14.7 memory leak fixes break references to event.target to Warn when reading from event which has been returned to the pool Jan 29, 2016

kentcdodds added a commit to kentcdodds/react that referenced this issue Jan 30, 2016

kentcdodds added a commit to kentcdodds/react that referenced this issue Jan 30, 2016

kentcdodds added a commit to kentcdodds/react that referenced this issue Feb 16, 2016

@jimfb jimfb closed this in 6312852 Feb 18, 2016

alunyov pushed a commit to alunyov/react that referenced this issue Mar 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment