setState() in componentDidMount() should flush synchronously even with createBatch() #12466
Conversation
gaearon
changed the title
| do { | ||
| performWorkOnRoot(root, expirationTime, false); | ||
| findHighestPriorityRoot(); | ||
| } while (nextFlushedRoot === root && nextFlushedExpirationTime === Sync); |
There was a problem hiding this comment.
Loops are scary. Are we sure we need this?
There was a problem hiding this comment.
Typically performWorkOnRoot is called inside a loop that finds next thing to work on until there's none.
flushRoot is special because it wants to flush "just one" root. So it only called performWorkOnRoot once. This is why it didn’t flush setState in commit lifecycles (they're just like flushing the same root twice—if my mental model is right).
There could be more than one cascading update. So it seems like a loop is necessary.
I'm not sure those are the right exit conditions though.
There was a problem hiding this comment.
Overall I'm not feeling confident in this. There are other things performWork does that performWorkOnRoot doesn’t. For example scheduling a deferred callback for leftover work. I’m not sure if missing this here means it would get dropped. I wonder if we could reuse the rest of the scheduler code, but teach the scheduler the concept of the "current" batch which gets committed in isolation if it exists.
There was a problem hiding this comment.
Also not sure what would happen if we used flushSync to schedule another root inside a currently committed batch's lifecycle.
Call performSyncWork right after flushing the batch. Does effectively the same thing by reusing the existing function. Also added some comments.
…h createBatch() (facebook#12466) * Add a failing test for setState in cDM during batch.commit() * Copy pasta * Flush all follow-up Sync work on the committed batch * Nit: Use performSyncWork Call performSyncWork right after flushing the batch. Does effectively the same thing by reusing the existing function. Also added some comments. * Delete accidentally duplicated test
I think this is a bug but I'm not familiar enough with the batching API to be sure. This is what Alan bumped into with UFI2.
My attempt at fixing is in 7e3a38b.