-
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
[Fiber] Set DOM attributes #7992
Conversation
I was under the impression that Injection was going to go away with Fiber (not sure the exact implications for how much of the DOM renderer would need to be rewritten or forked) |
Longer term that would be great IMO but shorter term we need to as many tests passing as possible while we simultaneously maintain both reconcilers. I don't care much either way, and I could just copy and paste those files and give them different names. |
Some of the event system and DOM properties injection will remain for now. However, you don't need them to implement a generic renderer. |
Rebased. |
oldProps: Props | null, | ||
newProps: Props, | ||
) { | ||
// TODO: associate DOM nodes with components for ReactPerf |
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.
Trying to understand how some things work, it looks like the ReactDOMComponentTree
logic was hoisted out of ReactDOMProperties
as a stepping stone to this TODO.
If I understand correctly: had this change not hoisted ReactDOMComponentTree
out of the (set|delete)ValueForProperty
calls the tests would fail. With this change a subset of the tests and functionality pass, though the ReactPerf
and ReactDevtools
would still not be able to take advantage of this.
Is this an okay-ish understanding or am I off?
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.
This is right. (I also was the person who put that DOMComponentTree thing into DOMOperations during ReactPerf rewrite because I was too lazy to pass debugID explicitly and now it's my time to pay for this.)
Per discussion, we'll fix small things (like composite tests wanting |
This builds on top of #7941 and adds basic support for setting DOM attributes.
Commit for review: 5a5bb86.
Test plan:
examples/fiber/index.html
now can putclassName
on adiv
:examples/basic/index.html
still works both with regular and minified builds.There were a couple of issues:
DOMProperty
so I had to injectReactDefaultInjection
which brought a bunch of the non-Fiber renderer into the Fiber build. I don't see a simpler workaround right now because we want to keep tests that testReactDOMFiber
alongside non-FiberyReactDOMServer
. Those are tricky to fix if we let bothReactDOMFiber
andReactDOMServer
inject their own stuff independently. Another plausible option is to completely fork the files but it seemed excessive.DOMPropertyOperations
fromReactDOMComponentTree
so I added an explicitdebugID
argument to all methods.This PR does include not support for events, web components, or style attribute. I figured I wouldn't waste time on this unless we agreed the approach is correct.
Fiber before this PR:
Fiber after this PR: