Be defensive when calling methods of a SyntheticEvent inside the pool

[slorber]

As it is known (but not documented), React's SyntheticEvent is pooled.

This is confusing for many users as they don't understand why the event starts to behave strangely when used in an async callback, like inside a setTimeout, a setState or a render callback.

There has already been an attempt to solve this problem here: #1664

The code of SyntheticEvent's default methods is:

  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;
  },

It may make sense to be more defensive because calling event.preventDefault() on a pooled event will raise can't call preventDefault on null.

It would be more useful to add a check like:

if ( this.nativeEvent ) throw new Error("Baaad you are using a pooled event!!!");

[slorber]

To illustrate the problem in not being defensive, I would give you this commit that has made it into production as a workaround in React-tappable:

JedWatson/react-tappable@a822fe9

event.preventDefault = function(){}; 

Basically the user is trying to assign an empty preventDefault function to a SyntheticEvent that as been put back in the pool (the call to persist was from another PR at the same time). And we were basically 3 concurrent PR to try different solutions to the exact same problem.

[nmn]

As the person who made the PR in a panic, I did find the code that broke in react itself.
the preventDefault method tries to call the method on this.nativeEvent.

A simple if check for this.nativeEvent in the method, and a no-op if null would be a great fix to this problem. Similar to how setState is handled on an unmounted component.

[slorber]

@nmm I think React should not swallow this error and be fail fast, because this does not make any sense to preventDefault in an async callback (because the default already has been applied), React should rather throw an error.

But this is only my opinion, because the browser behavior with dom events is not fail fast and swallow that bad usage of preventDefault. I mean the browser does not throw an error when we do:

setTimeout(function() {
    e.preventDefault();
},0)

It simply has no effect (like what you suggest)

I don't like this and would rather change the behavior of both React and the browser, but I don't think I can do much on my own...

Maybe at least React could issue a warning in DEV env?

[jimfb]

Yeah, I think warning is appropriate here.

[nmn]

A warning would be great!

[slorber]

Just wonder what kind of message should be put into this warning. If event pooling is not going to be documented (will it?), it may be strange to explain the pooling system inside a warning no?

[jimfb]

Yes, event pooling should/will be documented. Feel free to submit a PR.

We should add a warning to let the user know that the event has been returned to the pool and so invoking methods on it makes no sense. Once the docs are written, we can add a link to the end of the warning.

[edvinerikson]

Got a PR for this! Would really appreciate some feedback. #4635