Update Dirty Components in Mount Ordering

[yungsters]

Summary:
Currently, ReactUpdates updates dirty components in increasing order of mount depth. However, mount depth is only relative to the component passed into React.render. This breaks down for components that invoke React.render as an implementation detail because the child components will be updated before the parent component.

This fixes the problem by using the order in which components are mounted (instead of their depth). The mount order transcends component trees (rooted at React.render calls).

Reviewers: @sebmarkbage @zpao

Test Plan:
Ran unit tests successfully:

npm run jest

[syranide]

#1853 related? (components are notified of unmount in reverse order)

[syranide]

A few things stand out for me;

1. Reconciliation is now O(n log n) and not O(n) as it was... or is it O(n + m log m) perhaps? With n being the number of updated components and m being the number of updated roots. The op cost is low, but simply using radix sort instead should give us O(n) back for free I would think? (Oops, radix sort only applies if we sort on _mountDepth)
2. Is [2, 1, 0] really the correct order? Intuitively it seems as if it should be the reverse for mount.
3. _mountOrder seems like a weird solution, it is not compatible with reparenting (if that ever happens). It seems to me that simply sorting by _mountDepth would yield all the same benefits and components that rely on mounting order of siblings are inherently flawed... ?

[yungsters]

1. How does this change the running time of reconciliation? The only thing that changes is the order of updates.
2. Yes, [2, 1, 0] is logged by components in order of the hierarchy across render trees. (The tree that renders the component logging 2 contains the tree that renders the component logging 1, etc.)
3. Correct, this would not work with re-parenting. In order to work with re-parenting, we would have to add bookkeeping for the mount depth of all render trees. I'm not even sure what this would look like.

Relying on _mountDepth works fine within a render tree, but they are only scoped to that render tree. It breaks down when you have multiple render trees.

Due to the way that ReactUpdates re-orders all updates regardless of the order that the updates occurred, an application that correctly updates component state in the correct order will still break. In other words, the use of ReactUpdates causes all components to rely on the mounting order.

[syranide]

1. Sort is O(n log n) which is obviously worse than O(n) which I believe the rest of React rendering is, or did I misunderstand you? Each comparison is obviously super-cheap and unlikely to be measurable in the end though, so yeah not a big issue.
2. Ah ofc, [2, 1, 0] refers to count and not _mountOrder, then it all makes sense 👍

I see what you're saying about render trees now. But, technically, this is only a partial solution then. If we consider that toElement will make its appearance one day (or if you reparent DOM nodes or render to a detached node), then there's no guarantee that the parent render tree is rendered before the child, but obviously unlikely.

Hmm, could we not assign each render tree an order/depth instead and sort by: parent tree depth/order > component mount depth. Then we get the necessary render tree ordering. We could then also recompute the render tree order/depth each render (traverse the DOM parents, should be cheap) then we're guaranteed to always apply them in the correct order, but that could simply be forbidden I guess (and cross render tree component reparenting doesn't sound sane).

EDIT: The upside to sorting only by render tree depth/order is that we could use radix sort and make it O(n).

EDIT: All things said, your solution does make a lot of sense seing as there's no obvious golden ticket.

[sebmarkbage]

This is a very interesting artifact you've found. I like it. The only issue is that this algorithm doesn't parallelize well but I guess it can be bucketed.

Can we get a unit test of a use case which this fixes? Reconciliation order isn't part of the public contract because they're expected to be side-effect free but I'm guessing that something is incorrect without this fix? Or is it just a perf optimization? If so, the test should test that.

I suspect that this solution is incomplete since we don't treat rerenders as part of a subtree. E.g. events don't propagate etc. What other bugs could there be remaining?

[sebmarkbage]

This was accidentally closed because the branch is gone. However, the assumption will also start breaking down since with this feature:

https://github.com/reactjs/react-future/blob/master/04%20-%20Layout/02%20-%20Layout%20Components.js#L14

Children can be mounted before their parents. This won't really affect new render roots but everything up to that point.

refs and other things are already broken in batches across roots anyway. We need a more wholistic solution for this.

[yungsters]

Yup, sorry I am a n00b at GitHub. It's back up here: #2570