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

Detect React roots created by createRoot #14682

Closed

Conversation

philipp-spiess
Copy link
Contributor

@philipp-spiess philipp-spiess commented Jan 24, 2019

Fixes #14535
Related to #13778
Alternative to #14681

As @gaearon already noted, we can not rely on a container node having a _reactRootContainer to detect a React Root since the createRoot() API will not set it.

Furthermore, the createRoot() API is currently only setting a property on the container in DEV mode.

We could:

  1. Set a property in prod as well.
  2. Pass in more information into the appendChildToContainer() config. (Detect React roots by checking the parent fiber #14681)

This PR is an attempt to implement 1. It feels better than the other approach since we don't need to change the reconciler API.

Fixes facebook#14535
Related to facebook#13778
Alternative to facebook#14681

As @gaearon already noted, we can not rely on a container node having a
`_reactRootContainer` to detect a React Root since the `createRoot()`
API will not set it.

Furthermore, the `createRoot()` API is currently only setting a property
on the container in DEV mode.

We could:

 1. Set a property in prod as well.
 2. Pass in more information into the `appendChildToContainer()` config.

This PR is an attempt to implement 1. It feels better than [the other
approach](facebook#14681) since we don't
need to change the reconciler API.
@acdlite
Copy link
Collaborator

acdlite commented Jan 24, 2019

I would prefer to avoid relying on an expando

@acdlite
Copy link
Collaborator

acdlite commented Jan 24, 2019

we don't need to change the reconciler API

This is not necessarily a feature. The reconciler API might be flawed. If so, we shouldn't hack around it. Maybe the solution is to have separate methods for portals and roots.

@philipp-spiess
Copy link
Contributor Author

@acdlite Mhhhm it feels bad to change the reconciler API for this weird Safari bug but OTOH we only support portals for DOM right now, right? So perhaps there are differences between appending to a root and a portal and we just haven't noticed it yet. I'll update the other PR as well so we can compare both ideas.

@acdlite
Copy link
Collaborator

acdlite commented Jan 24, 2019

Mhhhm it feels bad to change the reconciler API for this weird Safari bug but OTOH we only support portals for DOM right now, right?

Yeah I'm not saying we should definitely change the reconciler API, I only want to make sure we consider the possibility that this could come up again, and that it's generally useful for the renderer to distinguish between roots and portals.

@stale
Copy link

stale bot commented Jan 10, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jan 10, 2020
@stale
Copy link

stale bot commented Jan 17, 2020

Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you!

@stale stale bot closed this Jan 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Resolution: Stale Automatically closed due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Concurrent mode iOS hover behaviour
3 participants