[Timers] Batch setImmediate handlers

[ide]

Wraps the setImmediate handlers in a batchUpdates call before they are synchronously executed at the end of the JS execution loop.

Test Plan: Added two setImmediate calls to componentDidMount in UIExplorerApp. Each handler calls setState, and componentWillUpdate logs its state. With this diff, we can see the state updates are successfully batched.

componentDidMount() {
  setImmediate(() => {
    console.log('immediate 1');
    this.setState({a: 1});
  });
  setImmediate(() => {
    console.log('immediate 2');
    this.setState({a: 2});
  });
},

componentWillUpdate(nextProps, nextState) {
  console.log('componentWillUpdate with next state.a =', nextState.a);
},

Before:

"immediate 1"
"componentWillUpdate with next state.a =", 1
"immediate 2"
"componentWillUpdate with next state.a =", 2

After:

"immediate 1"
"immediate 2"
"componentWillUpdate with next state.a =", 2

Addresses the batching issue in #1232. cc @vjeux @spicyj

[tadeuzagallo]

I was looking into it before, but I thought on doing it on processBatch(), since the other functions basically don't get called directly...
You could do something like:

var flushedQueue;
React.batchedUpdates(() => {
  flushedQueue = guardReturn(...);
});
return flushedQueue;

[ide]

I realized batchedUpdates needs to be inside of the guard. Update coming.

[ide]

Looking some more, the current diff appears correct to me. Both batchedUpdate calls (the one in processBatch and the new one in _flushQueueUnguarded) run inside of the guard so errors should be handled correctly.

It's easy to reduce the batching to one batch by moving JSTimersExecution.callImmediates() inside of processBatch, but if RN's semantics are intended to match the web's in this case, I think we actually want two separate batchedUpdate calls so that the setImmediate handlers run after the native->JS calls.

[sophiebits]

It is unintuitive to me that setImmediate calls would get batched together into the same React reconciliation. I'll have to think about it some more.

[ide]

@spicyj totally understand. What if they were process.nextTick calls? The motivation behind this diff is to batch the following updates:

var resolve1, resolve2;
new Promise((resolve) => resolve1 = resolve).then(() => {
  this.setState({...});
});
new Promise((resolve) => resolve2 = resolve).then(() => {
  this.setState({...});
});

// Resolve both promises in the same run of the event loop
resolve1();
resolve2();

Promises internally schedule a microtask (in RN this currently means setImmediate) but we want to batch the updates.

One catch is that if we use native Promises then they won't call setImmediate nor nextTick so maybe this diff isn't the best approach anyway. Feel free to take or reject it.

[vjeux]

@ide we can't use native promises as they don't respect React Native scheduling with the batch, so I don't think that's a big issue anyway

[vjeux]

Actually, assigning to @spicyj since he has some thoughts on the matter.

[sophiebits]

I think this is all right.

[sophiebits]

@facebook-github-bot import

plz

[vjeux]

@facebook-github-bot import

[vjeux]

strange that it didn't work for you @spicyj

[facebook-github-bot]

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/439269476233905/int_phab to review.

[sophiebits]

This got reverted internally. Will try to re-land.

[sophiebits]

@ide Anything change here? Still had this on my (long) list of things to do.

[ide]

@spicyj this issue should be open. I must have closed it when rebasing since it was merged once before being reverted. Reapplied the commit on top of master and retested.

[ide]

Closing this because setImmediates were batched in 4c74f01.