-
Notifications
You must be signed in to change notification settings - Fork 47.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Move child recursion to ReactReconciler #6011
Conversation
@mridgway updated the pull request. |
I'm not sure what the long-term goal is but maybe @sebmarkbage can confirm if this is on the right track. ReactReconciler should not have any DOM-specific logic in it, so nothing about HTML like you currently have because other environments don't have any analogous concept. I agree with your note in #4012 (comment) that the lazytree/reconciler coupling here feels off. I think we shouldn't need to add an additional injection point for this. Maybe we can instead create the children before each parent instance and then pass the mount images (or instances?) to mountComponent so that the component implementation can handle the DOM operations/etc. Passing pre-instantiated/resolved children to the parent moves closer to what @sebmarkbage and I have been talking about. (The order here shouldn't be observable as long as composites still happen in the same order.) |
Sorry about the delay of review. I've been reading it on and off. |
@spicyj Thanks for the feedback. I'll take a stab at pre-instantiation of children and update the PR this week. |
I'm just getting back around to looking at this. @spicyj I'm trying to get my head around what pre-instantiating the children would look like. I created the following hierarchies of what I understand of how React currently works, what my PR is proposing, and then what you are proposing: Current Master
PR Proposal
My interpretation of spicyj Proposal
After getting lost in the weeds for a while, I found the Reconciler needs to know whether the component is Native or Composite so that it knows whether or not to pass the current component as the The I also tried calling the If the Reconciler had a flag to know whether the component was native vs. composite then I think it would be easier for it to know which |
I think it is probably rational for the reconciler to understand native vs composite components. That is also how it would distinguish whether it should instantiate any passed children into components vs. just passing the elements down in |
This is a PR for ideas that @sebmarkbage recommended in #4012.
The idea is to centralize the recursion logic so that React is more pluggable for separate renderers and to support streaming in the future. I am hoisting all of the
ReactReconciler
calls out ofReact*Component
s and implementing the recursion inside ofReactReconciler.mountComponent
and other methods. This will affect theReact*Component
interface and implementation due to the need for post-recursion logic for things like event queueing.From my analysis these are the spots that need to be addressed and the progress of this PR with respect to each:
ReactCompositeComponent
mountComponent
Mount queue: componentDidMount (post-recursion)
receiveComponent
Mount queue: componentDidUpdate (post-recursion)
unmountComponent
ReactDOMComponent
mountComponent
Mount queue: trapBubbledEventsLocal (pre-recursion), putListener (pre-recursion), focusDOMComponent (post-recursion)
receiveComponent
Mount queue: putListener (pre-recursion), postUpdateSelectWrapper (post-recursion)
unmountComponent
ReactReconciler
Also,
ReactDOMComponent
usesDomLazyTree
objects that the reconciler would need to recognize. In order to keepReactReconciler
decoupled, there will need to be a generic interface for trees and a plugin for handlingDomLazyTree
specifically.mountComponent
LazyTree
injectionpostMount
onReact*Component
for implementing post-recursion event queueingreceiveComponent
postUpdate
onReact*Component
for implementing post-recursion event queueingunmountComponent
postUnmount
for implementing post-recursion event removalTesting
It would be good to get thoughts on whether this is going down the right path or if there's duplicate/conflicting work that is currently underway. So far I have only implemented the
mountComponent
path and tested against the example applications, but will continue the work for updates and unmounts as well.