-
Notifications
You must be signed in to change notification settings - Fork 49.6k
[Fiber] Reorganize files for DOM renderer to make overlap between fiber/stack clearer #8086
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
Conversation
We don't use this indirection. It'll easier to break these apart without it.
This splits DefaultInjection into one with all the properties and event configuration and a separate one for the things that are relevant to he stack reconciler. That way we can reuse the property and event system for Fiber without pulling in all the other stuff.
This is not a generic plugin order. It's DOM specific. Unlike say DefaultBatchingStrategy which is used by Native too.
22a2e08
to
00b691b
Compare
* of patent rights can be found in the PATENTS file in the same directory. | ||
* | ||
* @providesModule ReactDefaultInjection | ||
* @providesModule ReactDOMInjection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iamdustan This file might be of interest to you. It is the remainder of the injection system. At least in the first version of Fiber, but we can probably get rid of these completely at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah beautiful. Thanks for the callout! I’ll get a chance to look deeper later this week 😄
ReactDefaultInjection.inject(); | ||
var ReactDOMInjection = require('ReactDOMInjection'); | ||
ReactDOMInjection.inject(); | ||
var ReactDOMStackInjection = require('ReactDOMInjection'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the require statement here by ReactDOMStackInjection
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. Thanks. Updated. But I guess I don't technically need it to pass this test. Will revisit when we try to make this test work with Fiber.
👍 I like the change of It appears to me like the |
Just a spectator but I think confusion around the injection was one of the biggest things that threw me off when first contributing. This is fantastic. |
This is just moving a bunch of DOM files. It moves things into dom/stack and dom/fiber respectively. The dom/stack folder remains split into client/server. Mainly the shared folder is now my best guess for files that we can reuse in fiber. Everything else will get forked or reimplemented. Also moved the event system out of renderers/shared/stack and into renderers/shared/shared.
I verified that I was able to build Fiber with the |
Accepting to unblock, will properly review tomorrow morning. |
lgtm |
Read through, all looks good. |
I split the injection so that we can continue using the event/property system from fiber without pulling in everything.
I also reorganize files in my best guess for which files we can reuse instead of forking.