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

[Fiber] Set DOM attributes #7992

Closed
wants to merge 1 commit into from
Closed

[Fiber] Set DOM attributes #7992

wants to merge 1 commit into from

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Oct 17, 2016

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 put className on a div:

screen shot 2016-10-17 at 19 24 18

examples/basic/index.html still works both with regular and minified builds.

There were a couple of issues:

  • I needed DOMProperty so I had to inject ReactDefaultInjection 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 test ReactDOMFiber alongside non-Fibery ReactDOMServer. Those are tricky to fix if we let both ReactDOMFiber and ReactDOMServer inject their own stuff independently. Another plausible option is to completely fork the files but it seemed excessive.
  • I had to decouple DOMPropertyOperations from ReactDOMComponentTree so I added an explicit debugID 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:

Test Suites: 73 failed, 46 passed, 119 total
Tests:       712 failed, 3 skipped, 736 passed, 1451 total

Fiber after this PR:

Test Suites: 73 failed, 46 passed, 119 total
Tests:       690 failed, 3 skipped, 758 passed, 1451 total

@iamdustan
Copy link
Contributor

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)

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 17, 2016

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.

@sebmarkbage
Copy link
Collaborator

Some of the event system and DOM properties injection will remain for now. However, you don't need them to implement a generic renderer.

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 17, 2016

Rebased.

oldProps: Props | null,
newProps: Props,
) {
// TODO: associate DOM nodes with components for ReactPerf
Copy link
Contributor

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?

Copy link
Collaborator Author

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

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 18, 2016

Per discussion, we'll fix small things (like composite tests wanting className) on a case by case basis, and then do a big pass for DOM/events stuff.

@gaearon gaearon closed this Oct 18, 2016
@gaearon gaearon deleted the fiber-dom-easy branch October 18, 2016 18:58
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.

4 participants