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

Injection of ReactCompositeComponent implementation #4012

Closed
mridgway opened this issue Jun 3, 2015 · 4 comments
Closed

Injection of ReactCompositeComponent implementation #4012

mridgway opened this issue Jun 3, 2015 · 4 comments

Comments

@mridgway
Copy link
Contributor

mridgway commented Jun 3, 2015

In researching a separate server implementation, I have found that being able to inject different component types in to instantiateReactComponent allows me to bypass much of the reconciliation logic that is not needed on the server. Via ReactNativeComponent, instantiateReactComponent most of the types are injectable with the exception of ReactCompositeComponent.

I've also noticed that a lot of the component injection is spread in different two files: ReactEmptyComponent and ReactNativeComponent. Neither of these seem like a good place to add CompositeComponent injection.

These injection containers also contain a default implementation for many of their methods which could bloat package sizes if the defaults are overridden as they would likely be for the server.

I would propose to have a ReactComponentFactory that contains the instantiateReactComponent method and provides the injection for each type of component. Defaults implementations would be provided only by ReactDefaultInjection, which would likely also have a derivative ReactServerDefaultInject.

This would also allow removal of this circular dependency wrapper: https://github.com/facebook/react/blob/master/src/renderers/shared/reconciler/instantiateReactComponent.js#L24

@jimfb
Copy link
Contributor

jimfb commented Jul 18, 2015

My intuition is: now that 0.14 is splitting renderers out from core, it should be easier/better to just have a separate renderer for server side. A substantial portion of the logic can be simplified/removed, streaming can be implemented, skipping component instantiation becomes a possibility (ok, maybe that one is hard/questionable), etc. Things that are common to client and server can be moved into shared, and things that are different can be forked. I tend to like that strategy better than injection/factories, because I find it more difficult to reason about the flow of control with the latter patterns. That said, I'm curious what @sebmarkbage thinks.

@sebmarkbage
Copy link
Collaborator

@jimfb The problem is when you want to fork something in the middle and don't want to fork everything above it.

We do want a system for injections but I would like it to become statically resolved. E.g. through overrides of requires. Then the shared code would be copied into each renderer build (i.e. not shared on npm), then minified and dead-code eliminated as a unit.

@sebmarkbage
Copy link
Collaborator

@mridgway The current set up is really ad hoc and messy as part of incremental refactoring. :)

So here's the plan that I'm shooting for...

I would for renderers to be a two layer system:

  • The generic React reconciler part (which in turn could be split into a server side and a client side).
  • The environment specific part (e.g. the DOM specific pieces, react-native specific pieces, react-art, etc. etc.)

This should make it trivial to target a new imperative API by just plugging in the environment specific parts into the generic reconciler part.

There is currently a necessary cycle between ReactCompositeComponent and ReactDOMComponent because ReactDOMComponent causes a recursion which may lead back to ReactCompositeComponent.

We solve this using a dynamic hacky injection workaround, which is unfortunate for static analysis and VM optimization reasons.

However, that recursion is an implementation detail that we would like to get out of the environment specific logic. I.e. the DOM specific code shouldn't do any recursion. That should be part of the generic React reconciler.

That way the entire cycle can move into ReactReconciler as a unit. No more module cycles.

@mridgway
Copy link
Contributor Author

mridgway commented Feb 6, 2016

I wanted to circle back on this as I think it blocks some other changes that I wanted to look into (streaming and async).

I started implementing your suggestion of moving the recursion up to ReactReconciler: master...mridgway:reconcilerRecursion (tests not updated yet, but basic examples working)

The unfortunate thing is that the DOMLazyTree calls depended on the mounted images to be executed within the components, so I had to move that logic into ReactReconciler where the actual mounting is happening. I think the best way to resolve this would be to move all of the transaction.useCreateElement forks out of React*Component and have their respective mountComponents return simpler meta objects that the ReactReconciler would use to call implementation specific adapters.

Any other thoughts on the best way to decouple the ReactReconciler from the DOMLazyTree methods?

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

No branches or pull requests

3 participants