Skip to content
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

Closed
wants to merge 4 commits into from

Conversation

mridgway
Copy link
Contributor

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 of React*Components and implementing the recursion inside of ReactReconciler.mountComponent and other methods. This will affect the React*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)

  • hoist ReactReconciler calls
  • Add postMount interface to handle post-recursion

receiveComponent

Mount queue: componentDidUpdate (post-recursion)

  • hoist ReactReconciler calls
  • Add postUpdate interface to handle post-recursion

unmountComponent

  • hoist ReactReconciler calls

ReactDOMComponent

mountComponent

Mount queue: trapBubbledEventsLocal (pre-recursion), putListener (pre-recursion), focusDOMComponent (post-recursion)

  • hoist ReactReconciler calls
  • Add postMount interface to handle post-recursion

receiveComponent

Mount queue: putListener (pre-recursion), postUpdateSelectWrapper (post-recursion)

  • hoist ReactReconciler calls
  • Add postUpdate interface to handle post-recursion

unmountComponent

  • hoist ReactReconciler calls

ReactReconciler

Also, ReactDOMComponent uses DomLazyTree objects that the reconciler would need to recognize. In order to keep ReactReconciler decoupled, there will need to be a generic interface for trees and a plugin for handling DomLazyTree specifically.

mountComponent

  • implement recursion
  • attachRefs queue (pre: must be before other queued methods)
  • implement generic LazyTree injection
  • call postMount on React*Component for implementing post-recursion event queueing

receiveComponent

  • implement recursion
  • call postUpdate on React*Component for implementing post-recursion event queueing

unmountComponent

  • implement recursion
  • call postUnmount for implementing post-recursion event removal

Testing

  • Fix tests that are affected by internal API changes

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.

@facebook-github-bot
Copy link

@mridgway updated the pull request.

@jimfb
Copy link
Contributor

jimfb commented Feb 10, 2016

@sebmarkbage @spicyj

@sophiebits
Copy link
Collaborator

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.)

@sebmarkbage
Copy link
Collaborator

Sorry about the delay of review. I've been reading it on and off.

@mridgway
Copy link
Contributor Author

@spicyj Thanks for the feedback. I'll take a stab at pre-instantiation of children and update the PR this week.

@mridgway
Copy link
Contributor Author

mridgway commented Mar 2, 2016

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

component = instantiateReactComponent(element);
ReactReconciler.mountComponent(component);
    ReactCompositeComponent.mountComponent(component);
        childElement = element.render();
        childComponent = instantiateReactComponent(childElement);
        ReactReconciler.mountComponent(childComponent);
            ReactCompositeComponent.mountComponent(component);
                childElement = childElement.render();
                childComponent = instantiateReactComponent(childElement);
                ReactReconciler.mountComponent(childComponent);
                    ...

PR Proposal

component = instantiateReactComponent(element);
ReactReconciler.mountComponent(component);
    childComponent = ReactCompositeComponent.mountComponent(component);
        childElement = element.render();
        return instantiateReactComponent(childElement);
    ReactReconciler.mountComponent(childComponent);
        childComponent = ReactCompositeComponent.mountComponent(component);
            childElement = childElement.render();
            return instantiateReactComponent(childElement);
        ReactReconciler.mountComponent(childComponent);
            ...

My interpretation of spicyj Proposal

component = instantiateReactComponent(element);
// new preMount API on React*Component? This is currently part of mountComponent
childElement = element.render();
childComponent = instantiateReactComponent(childElement);
// end preMount
ReactReconciler.mountComponent(component, childComponent);
    ReactCompositeComponent.mountComponent(component, childComponent);
    childOfChildElement = childElement.render();
    childOfChildComponent = instantiateReactComponent(childOfChildElement);
    ReactReconciler.mountComponent(childComponent, childOfChildComponent)
        ...

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 nativeParent or to propagate the current nativeParent.

The preMount step could be called from within React*Component.preMount so that they could pass the nativeParent on its own, but that seems like it negates the benefits of moving recursion into the Reconciler.

I also tried calling the preMount step from within React*Component.mountComponent but I lose access to the premounted components that need to be passed to the children's mountComponent since mountComponent still needs to return the raw result (strings, LazyTrees, etc.).

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 nativeParent should be passed in. Do you know if there is a way to do this right now?

@mridgway mridgway closed this Mar 3, 2016
@sophiebits
Copy link
Collaborator

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.props.children.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants