Skip to content
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

Document that events are pooled #3657

Closed
andersekdahl opened this issue Apr 13, 2015 · 7 comments
Closed

Document that events are pooled #3657

andersekdahl opened this issue Apr 13, 2015 · 7 comments

Comments

@andersekdahl
Copy link

I ran into an issue where I had an event handler that called setState by passing in a function and accessing e.target.value inside that function. It seems that Reacts synthetic events are "garbage collected" after the event handler has finished executing, which gave me an error when I tried to access value of undefined (since all properties were removed from the event object).

This is fine, and easy to fix by destructuring the event as in the argument (({target: {value}}) => this.setState(...) instead of e => this.setState(...)), but it should probably be documented.

@bloodyowl
Copy link
Contributor

event objects are pooled. you can use event.persist() to preserve the object.

I agree this should be documented though

@andersekdahl
Copy link
Author

Ah, interesting, didn't know that.

@andersekdahl andersekdahl changed the title Document that events are "garbage collected" Document that events are pooled Apr 13, 2015
@benmosher
Copy link

👍, this surprised me, too. Plus, I'm curious about the motivation 😄

@jimfb
Copy link
Contributor

jimfb commented Apr 20, 2015

I think the motivation was performance (it is expensive to create lots of short lived objects, puts lots of pressure on the javascript garbage collector). onClick wouldn't be a big deal, but a mouse-move event could fire very often.

Having said that, this issue keeps coming up as a surprising/confusing part of the framework (lots of people spend lots of time debugging an issue before they realize the problem), so it might make sense to consider changing this. cc @zpao @spicyj

But yes, we should document it if we are going to keep this behavior.

@syranide
Copy link
Contributor

@JSFB #1612 (comment)

@slorber
Copy link
Contributor

slorber commented Jul 28, 2015

I also run into this issue sometimes.

@jimfb another thing that probably should be done is good error/warning messages in case of using SyntheticEvent's methods while the object has been put back in the pool.

For example I had a case where I tried to call preventDefault on a SyntheticEvent that was put back and got a "can't call preventDefault on null" because of the following code:

  preventDefault: function() {
    this.defaultPrevented = true;
    var event = this.nativeEvent;
    if (event.preventDefault) {
      event.preventDefault();
    } else {
      event.returnValue = false;
    }
    this.isDefaultPrevented = emptyFunction.thatReturnsTrue;
  },

  stopPropagation: function() {
    var event = this.nativeEvent;
    if (event.stopPropagation) {
      event.stopPropagation();
    } else {
      event.cancelBubble = true;
    }
    this.isPropagationStopped = emptyFunction.thatReturnsTrue;
  },

I think the SyntheticEvent methods should rather be defensive and explain the pooling when the user face it

slorber referenced this issue in JedWatson/react-tappable Jul 29, 2015
calling preventDefault on the event received in onTap is currently broken on iOS. This fixes that.
slorber added a commit to slorber/react-tappable that referenced this issue Jul 29, 2015
The solution is to fire the tap event always synchronously (already fixed).
So the React event pooling can behave as usual.

Removed the attempt to assign an empty function to preventDefault, because it does not make sense to assign a function to a SyntheticEvent that has been put back in the pool. It should be the role of React to be defensive agaiin using preventDefault on events that are already put back in the pool. (See facebook/react#3657 (comment) )
@zpao
Copy link
Member

zpao commented Aug 17, 2015

Closed with #4634

@zpao zpao closed this as completed Aug 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants