Call componentDidMount for children before componentDidUpdate for parent#506
Conversation
Previously the `componentDidUpdate` hook was called immediately after a component was updated but `componentDidMount` was deferred until the end of the render cycle in order to ensure that the corresponding DOM element was actually present in the document when the callback was invoked. The result was that after an update to a component which mounted new child component instances, the parent's `componentDidUpdate` would be called before the child's `componentDidMount`. This broke the assumptions of the React Dev Tools and possibly other projects. Fix the problem by flushing the queue of mount callbacks before executing any `componentDidUpdate` callbacks. Fixes preactjs#440 ---- In order for this to be "safe" (ie. preserve the invariant that `componentDidMount` is only called for connected [1] components), it must be the case that all new instances mounted before an update to an existing instance in a render cycle are connected before the update happens. When `componentDidUpdate` is called during a render cycle, there are two cases to consider for components in the mount queue: 1. The component is a descendant of the one being updated: A // Updated |- B // Mounted, pending `componentDidMount` callback 2. The component is part of a subtree rooted at a previous sibling of the one being updated: A |-B // Mounted, pending `componentDidMount` callback |-C // Updated Case (1) is OK because the `componentDidUpdate` callback is only called on connected components and only after any children have been diffed and added to the parent element, so (B) will be connected when (A) is updated. Case (2) is OK because the logic in `innerDiffNode` diffs each child in order and adds it to the parent, so (B) will be connected by the time (C) is updated. [1] "connected" - The component's root DOM element and all of its children have been created and connected to the host document.
|
Need as many eyes on this PR as we can. It passes tests, and @robertknight and I couldn't think of any reasons why it would be bad to flush early here. However, it's a major change to the deferral mechanism. Any comments, logs, inspections, etc are appreciated! |
|
I'm not sure I 100% get, but AFAIK |
That is correct. Whenever a component is mounted, preact puts the component on a queue to trigger the Previously this queue was always flushed at the very end of the render cycle. However here I'm relying on the fact that given the current rendering order, it should also be safe to flush the queue whenever we get to updating an existing component. In the original bug report, the structure was initially this: After a route change, the new structure looks like this: Previously, the |
|
Just reminding myself that this is still open. Need to verify some things and then merge. |
|
@robertknight I think this might fix an compat issue with React Bootstrap. |
|
Is the React Bootstrap issue filed anywhere? |
|
@robertknight Let's get this into the next 8 beta. Then we can just watch for reports of any lifecycle oddities. |
|
Just one thing: parent's componentDidUpdate, children' componentDidMount should be called after DOM created/updated. It seems that this PR has broken it. @developit |
Previously the
componentDidUpdatehook was called immediately after acomponent was updated but
componentDidMountwas deferred until the endof the render cycle in order to ensure that the corresponding DOM
element was actually present in the document when the callback was
invoked.
The result was that after an update to a component which mounted new
child component instances, the parent's
componentDidUpdatewould becalled before the child's
componentDidMount. This broke the assumptionsof the React Dev Tools and possibly other projects.
Fix the problem by flushing the queue of mount callbacks before
executing any
componentDidUpdatecallbacks. See the commit message for amore detailed explainer.
Fixes #440