-
Notifications
You must be signed in to change notification settings - Fork 49.8k
[Fiber] Set DOM attributes #7992
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
|
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.htmlnow can putclassNameon adiv:examples/basic/index.htmlstill works both with regular and minified builds.There were a couple of issues:
DOMPropertyso I had to injectReactDefaultInjectionwhich 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 testReactDOMFiberalongside non-FiberyReactDOMServer. Those are tricky to fix if we let bothReactDOMFiberandReactDOMServerinject their own stuff independently. Another plausible option is to completely fork the files but it seemed excessive.DOMPropertyOperationsfromReactDOMComponentTreeso I added an explicitdebugIDargument 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: