Skip to content

Alternate method of jumping to the root of the render tree. #4558

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

Closed
wants to merge 1 commit into from

Conversation

jimfb
Copy link
Contributor

@jimfb jimfb commented Aug 4, 2015

Alternate method of jumping to the root of the render tree.

#4511 is a nasty bug to fix, but the recent event work makes it slightly more likely that we'll hit this bug. This change helps mitigate the issue by avoiding the calls to ReactMount.getId() that do invariant checks, thus allowing us to tiptoe around the underlying issue.

On a side note, there are some thoughts that this might be slightly faster too, so it might even be a perf win :).

@jimfb
Copy link
Contributor Author

jimfb commented Aug 4, 2015

cc @sebmarkbage @spicyj

@@ -128,10 +128,10 @@ function handleTopLevelWithPath(bookKeeping) {
);

// Jump to the root of this React render tree
while (currentPathElementID !== newRootID) {
var container = ReactMount.findReactContainerForID(currentPathElementID);
var parent = ReactMount.getFirstReactDOM(container);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is the wrong one since it is walking the parentNode pointers.

sophiebits added a commit to sophiebits/react that referenced this pull request Aug 7, 2015
Looks like facebook#4558 isn't ready yet and we'd like an interim fix.
@jimfb jimfb force-pushed the jump-to-root-optimization branch from 4fb5e32 to 14e3a25 Compare August 11, 2015 01:35
@jimfb jimfb force-pushed the jump-to-root-optimization branch 2 times, most recently from 2b8a4db to 6781f29 Compare August 11, 2015 02:31
@jimfb jimfb force-pushed the jump-to-root-optimization branch from 6781f29 to 89f25c8 Compare August 11, 2015 02:34
@jimfb
Copy link
Contributor Author

jimfb commented Aug 11, 2015

@sebmarkbage Updated. Manually tested it in chrome and firefox, both with and without webcomponents. Also tested with the polyfill. All looks good now.

@sebmarkbage
Copy link
Collaborator

We should put this on hold: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/8_x0OHYQdx0

https://w3c.github.io/webcomponents/spec/shadow/#widl-Event-deepPath

This is getting renamed and/or deprecated. It seems like there might be edge cases where even deepPath isn't enough. I think we should keep this disabled in 0.14 until there is at least a second implementation and the spec stabilizes.

@sebmarkbage
Copy link
Collaborator

I think I've also found another edge case that we're not testing for.

Something like: <div><x-custom><div onClick={...}>...</div></x-custom></div>

Where the shadow DOM created by x-custom could've used <content /> or similar to render the child at a different path that doesn't line up with our expectations. Especially if that shadow DOM is also using React.

@thetaxman
Copy link

Someone please help me out with this

@gaearon
Copy link
Collaborator

gaearon commented Mar 26, 2016

It appears that the code path that was introduced in #4150 was later completely removed by f470cb8 so this no longer merges.

I am closing per @sebmarkbage’s comments above. If we plan to get back to this let’s create a separate issue describing the problem, and reach a consensus on how this should be handled.

@gaearon gaearon closed this Mar 26, 2016
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.

5 participants