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

[DevTools] Track all public HostInstances in a Map #30831

Merged
merged 6 commits into from
Sep 3, 2024

Conversation

sebmarkbage
Copy link
Collaborator

This lets us get from a HostInstance to the nearest DevToolsInstance without relying on findFiberByHostInstance and fiberToDevToolsInstanceMap. We already did the equivalent of this for Resources in HostHoistables.

One issue before was that we'd ideally get away from the fiberToDevToolsInstanceMap map in general since we should ideally not treat Fibers as stateful but they could be replaced by something else stateful in principle.

This PR also addresses Virtual Instances. Now you can select a DOM node and have it select a Virtual Instance if that's the nearest parent since the parent doesn't have to be a Fiber anymore.

However, the other reason for this change is that I'd like to get rid of the need for the findFiberByHostInstance from being injected. A renderer should not need to store a reference back from its instance to a Fiber. Without the Synthetic Event system this wouldn't be needed by the renderer so we should be able to remove it. We also don't really need it since we have all the information by just walking the commit to collect the nodes if we just maintain our own Map.

There's one subtle nuance that the different renderers do. Typically a HostInstance is the same thing as a PublicInstance in React but technically in Fabric they're not the same. So we need to translate between PublicInstance and HostInstance. I just hardcoded the Fabric implementation of this since it's the only known one that does this but could feature detect other ones too if necessary. On one hand it's more resilient to refactors to not rely on injected helpers and on hand it doesn't follow changes to things like this.

For the conflict resolution I added in #30494 I had to make that specific to DOM so we can move the DOM traversal to the backend instead of the injected helper.

@sebmarkbage sebmarkbage requested a review from hoxyq August 28, 2024 01:45
Copy link

vercel bot commented Aug 28, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 3, 2024 9:20pm

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Aug 28, 2024
@@ -1448,6 +1504,18 @@ export function attach(
fiberToFiberInstanceMap.delete(alternate);
}
}

// TODO: This is not enough if this Fiber was filtered since we don't end up
// untracking it. We could use a WeakMap but that doesn't work for Paper tags.
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 not really new. I discovered this was already an issue with leaking HostHoistables but it's really a blocker for landing this PR since it leaks DOM nodes if they're filtered which is way more common.

I can address this with a separate PR though which I think will need to start tracking some filtered Fiber Instances in some cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be fixed by just manually releasing host instance / resource if instance is filtered out?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea I'm just not sure how to structure it yet because we can't just recursively do it because we also recursively walk the Instance tree. I also have another need to track FiberInstances that are filtered to get the HostInstance from a VirtualInstance. So I was going to see where that landed first.

I won't land this until it has a solution though.

Copy link
Collaborator Author

@sebmarkbage sebmarkbage Sep 3, 2024

Choose a reason for hiding this comment

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

I went with this approach to manually release if they're filtered out:

a4ae4d6

This will work with #30865 because we can start from the filtered fiber instance and then untrack from there.

The shouldFilterFiber function isn't exactly always fast so it's not great to use it like this for every filtered instance if it's slow but in the case where it's just checking a tag it's ok.

@@ -1448,6 +1504,18 @@ export function attach(
fiberToFiberInstanceMap.delete(alternate);
}
}

// TODO: This is not enough if this Fiber was filtered since we don't end up
// untracking it. We could use a WeakMap but that doesn't work for Paper tags.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this be fixed by just manually releasing host instance / resource if instance is filtered out?

This lets us get from a HostInstance to the nearest DevToolsInstance
without relying on findNearestUnfilteredElementID and
fiberToDevToolsInstanceMap.
This conflict resolution is really only relevant to DOM renderers since
there's generally multiple of them per tree and this resolution is only
active for those anyway. Making this clear means we can change the
implementation details.
We don't need the dependency on findFiberByHostInstance now that we maintain
our own map of public instances.

This relies on the assumption that this is a DOM renderer but we could always
add support for more renderer types that need conflict resolution if they
happen.
If we have any filtered children, we need to untrack those as well.
@sebmarkbage sebmarkbage merged commit d1afcb4 into facebook:main Sep 3, 2024
184 checks passed
gnoff pushed a commit to vercel/next.js that referenced this pull request Sep 12, 2024
**breaking change for canary users: Bumps peer dependency of React from
`19.0.0-rc-7771d3a7-20240827` to `19.0.0-rc-94e652d5-20240912`**

[diff
facebook/react@7771d3a7...94e652d5](facebook/react@7771d3a...94e652d)

<details>
<summary>React upstream changes</summary>

- facebook/react#30952
- facebook/react#30950
- facebook/react#30946
- facebook/react#30934
- facebook/react#30947
- facebook/react#30945
- facebook/react#30938
- facebook/react#30936
- facebook/react#30879
- facebook/react#30888
- facebook/react#30931
- facebook/react#30930
- facebook/react#30832
- facebook/react#30929
- facebook/react#30926
- facebook/react#30925
- facebook/react#30905
- facebook/react#30900
- facebook/react#30910
- facebook/react#30906
- facebook/react#30899
- facebook/react#30919
- facebook/react#30708
- facebook/react#30907
- facebook/react#30897
- facebook/react#30896
- facebook/react#30895
- facebook/react#30887
- facebook/react#30889
- facebook/react#30893
- facebook/react#30892
- facebook/react#30891
- facebook/react#30882
- facebook/react#30881
- facebook/react#30870
- facebook/react#30849
- facebook/react#30878
- facebook/react#30865
- facebook/react#30869
- facebook/react#30875
- facebook/react#30800
- facebook/react#30762
- facebook/react#30831
- facebook/react#30866
- facebook/react#30853
- facebook/react#30850
- facebook/react#30847
- facebook/react#30842
- facebook/react#30837
- facebook/react#30848
- facebook/react#30844
- facebook/react#30839
- facebook/react#30802
- facebook/react#30841
- facebook/react#30827
- facebook/react#30826
- facebook/react#30825
- facebook/react#30824
- facebook/react#30840
- facebook/react#30838
- facebook/react#30836
- facebook/react#30819
- facebook/react#30816
- facebook/react#30814
- facebook/react#30813
- facebook/react#30812
- facebook/react#30811

</details>

---------

Co-authored-by: vercel-release-bot <infra+release@vercel.com>
hoxyq added a commit that referenced this pull request Sep 25, 2024
Full list of changes:
* refactor: data source for errors and warnings tracking is now in Store
([hoxyq](https://github.com/hoxyq) in
[#31010](#31010))
* fix: consider alternate as a key for componentLogsEntry when
inspecting raw fiber instance ([hoxyq](https://github.com/hoxyq) in
[#31009](#31009))
* Fix: profiling crashes #30661 #28838
([EdmondChuiHW](https://github.com/EdmondChuiHW) in
[#31024](#31024))
* chore: remove using local storage for persisting console settings on
the frontend ([hoxyq](https://github.com/hoxyq) in
[#31002](#31002))
* feat: display message if user ended up opening hook script
([hoxyq](https://github.com/hoxyq) in
[#31000](#31000))
* feat: expose installHook with settings argument from
react-devtools-core/backend ([hoxyq](https://github.com/hoxyq) in
[#30987](#30987))
* chore: remove settings manager from react-devtools-core
([hoxyq](https://github.com/hoxyq) in
[#30986](#30986))
* feat[react-devtools/extension]: use chrome.storage to persist settings
across sessions ([hoxyq](https://github.com/hoxyq) in
[#30636](#30636))
* refactor[react-devtools]: propagate settings from global hook object
to frontend ([hoxyq](https://github.com/hoxyq) in
[#30610](#30610))
* chore[react-devtools]: extract some utils into separate modules to
unify implementations ([hoxyq](https://github.com/hoxyq) in
[#30597](#30597))
* refactor[react-devtools]: move console patching to global hook
([hoxyq](https://github.com/hoxyq) in
[#30596](#30596))
* refactor[react-devtools]: remove browserTheme from
ConsolePatchSettings ([hoxyq](https://github.com/hoxyq) in
[#30566](#30566))
* feat[react-devtools]: add settings to global hook object
([hoxyq](https://github.com/hoxyq) in
[#30564](#30564))
* fix: add Error prefix to Error objects names
([hoxyq](https://github.com/hoxyq) in
[#30969](#30969))
* Add enableComponentPerformanceTrack Flag
([sebmarkbage](https://github.com/sebmarkbage) in
[#30960](#30960))
* fix[rdt/fiber/renderer.js]: getCurrentFiber can be injected as null
([hoxyq](https://github.com/hoxyq) in
[#30968](#30968))
* disable `enableSiblingPrerendering` in experimental channel
([gnoff](https://github.com/gnoff) in
[#30952](#30952))
* refactor[react-devtools]: initialize renderer interface early
([hoxyq](https://github.com/hoxyq) in
[#30946](#30946))
* Start prerendering Suspense retries immediately
([acdlite](https://github.com/acdlite) in
[#30934](#30934))
* refactor[Agent/Store]: Store to send messages only after Agent is
initialized ([hoxyq](https://github.com/hoxyq) in
[#30945](#30945))
* refactor[RendererInterface]: expose onErrorOrWarning and
getComponentStack ([hoxyq](https://github.com/hoxyq) in
[#30931](#30931))
* Implement getComponentStack and onErrorOrWarning for replayed Flight
logs ([sebmarkbage](https://github.com/sebmarkbage) in
[#30930](#30930))
* Use Unicode Atom Symbol instead of Atom Emoji
([sebmarkbage](https://github.com/sebmarkbage) in
[#30832](#30832))
* Improve Layering Between Console and Renderer
([sebmarkbage](https://github.com/sebmarkbage) in
[#30925](#30925))
* Add Map for Server Component Logs
([sebmarkbage](https://github.com/sebmarkbage) in
[#30905](#30905))
* Delete fiberToFiberInstanceMap
([sebmarkbage](https://github.com/sebmarkbage) in
[#30900](#30900))
* Add Flight Renderer ([sebmarkbage](https://github.com/sebmarkbage) in
[#30906](#30906))
* Refactor Error / Warning Count Tracking
([sebmarkbage](https://github.com/sebmarkbage) in
[#30899](#30899))
* [flow] Upgrade Flow to 0.245.2
([SamChou19815](https://github.com/SamChou19815) in
[#30919](#30919))
* Separate RDT Fusebox into single-panel entry points
([huntie](https://github.com/huntie) in
[#30708](#30708))
* Build Updater List from the Commit instead of Map
([sebmarkbage](https://github.com/sebmarkbage) in
[#30897](#30897))
* Simplify Context Change Tracking in Profiler
([sebmarkbage](https://github.com/sebmarkbage) in
[#30896](#30896))
* Remove use of .alternate in root and recordProfilingDurations
([sebmarkbage](https://github.com/sebmarkbage) in
[#30895](#30895))
* Handle reordered contexts in Profiler
([sebmarkbage](https://github.com/sebmarkbage) in
[#30887](#30887))
* Refactor Forcing Fallback / Error of Suspense / Error Boundaries
([sebmarkbage](https://github.com/sebmarkbage) in
[#30870](#30870))
* Avoid getFiberIDUnsafe in debug() Helper
([sebmarkbage](https://github.com/sebmarkbage) in
[#30878](#30878))
* Include some Filtered Fiber Instances
([sebmarkbage](https://github.com/sebmarkbage) in
[#30865](#30865))
* Track root instances in a root Map
([sebmarkbage](https://github.com/sebmarkbage) in
[#30875](#30875))
* Track all public HostInstances in a Map
([sebmarkbage](https://github.com/sebmarkbage) in
[#30831](#30831))
* Support VirtualInstances in findAllCurrentHostInstances
([sebmarkbage](https://github.com/sebmarkbage) in
[#30853](#30853))
* Add Filtering of Environment Names
([sebmarkbage](https://github.com/sebmarkbage) in
[#30850](#30850))
* Support secondary environment name when it changes
([sebmarkbage](https://github.com/sebmarkbage) in
[#30842](#30842))
* Increase max payload for websocket in standalone app
([runeb](https://github.com/runeb) in
[#30848](#30848))
* Filter Server Components
([sebmarkbage](https://github.com/sebmarkbage) in
[#30839](#30839))
* Track virtual instances on the tracked path for selections
([sebmarkbage](https://github.com/sebmarkbage) in
[#30802](#30802))
* Remove displayName from inspected data
([sebmarkbage](https://github.com/sebmarkbage) in
[#30841](#30841))
* chore[react-devtools/hook]: remove unused native values
([hoxyq](https://github.com/hoxyq) in
[#30827](#30827))
* chore[react-devtools/extensions]: remove unused storage permission
([hoxyq](https://github.com/hoxyq) in
[#30826](#30826))
* fix[react-devtools/extensions]: fixed tabs API calls and displaying
restricted access popup ([hoxyq](https://github.com/hoxyq) in
[#30825](#30825))
* feat[react-devtools]: support Manifest v3 for Firefox extension
([hoxyq](https://github.com/hoxyq) in
[#30824](#30824))
* Reconcile Fibers Against Previous Children Instances
([sebmarkbage](https://github.com/sebmarkbage) in
[#30822](#30822))
* Remove findCurrentFiberUsingSlowPathByFiberInstance
([sebmarkbage](https://github.com/sebmarkbage) in
[#30818](#30818))
* Track Tree Base Duration of Virtual Instances
([sebmarkbage](https://github.com/sebmarkbage) in
[#30817](#30817))
* Use Owner Stacks to Implement View Source of a Server Component
([sebmarkbage](https://github.com/sebmarkbage) in
[#30798](#30798))
* Make function inspection instant
([sebmarkbage](https://github.com/sebmarkbage) in
[#30786](#30786))
* Make Functions Clickable to Jump to Definition
([sebmarkbage](https://github.com/sebmarkbage) in
[#30769](#30769))
* Support REACT_LEGACY_ELEMENT_TYPE for formatting JSX
([sebmarkbage](https://github.com/sebmarkbage) in
[#30779](#30779))
* Find owners from the parent path that matches the Fiber or
ReactComponentInfo ([sebmarkbage](https://github.com/sebmarkbage) in
[#30717](#30717))
* [Flight/DevTools] Pass the Server Component's "key" as Part of the
ReactComponentInfo ([sebmarkbage](https://github.com/sebmarkbage) in
[#30703](#30703))
* Hide props section if it is null
([sebmarkbage](https://github.com/sebmarkbage) in
[#30696](#30696))
* Support Server Components in Tree
([sebmarkbage](https://github.com/sebmarkbage) in
[#30684](#30684))
* fix[react-devtools/InspectedElement]: fixed border stylings when some
of the panels are not rendered ([hoxyq](https://github.com/hoxyq) in
[#30676](#30676))
* Compute new reordered child set from the instance tree
([sebmarkbage](https://github.com/sebmarkbage) in
[#30668](#30668))
* Unmount instance by walking the instance tree instead of the fiber
tree ([sebmarkbage](https://github.com/sebmarkbage) in
[#30665](#30665))
* Further Refactoring of Unmounts
([sebmarkbage](https://github.com/sebmarkbage) in
[#30658](#30658))
* Remove lodash.throttle ([sebmarkbage](https://github.com/sebmarkbage)
in [#30657](#30657))
* Unmount by walking previous nodes no longer in the new tree
([sebmarkbage](https://github.com/sebmarkbage) in
[#30644](#30644))
* Build up DevTools Instance Shadow Tree
([sebmarkbage](https://github.com/sebmarkbage) in
[#30625](#30625))
* chore[packages/react-devtools]: remove unused index.js
([hoxyq](https://github.com/hoxyq) in
[#30579](#30579))
* Track DOM nodes to Fiber map for HostHoistable Resources
([sebmarkbage](https://github.com/sebmarkbage) in
[#30590](#30590))
* Rename mountFiberRecursively/updateFiberRecursively
([sebmarkbage](https://github.com/sebmarkbage) in
[#30586](#30586))
* Allow Highlighting/Inspect HostSingletons/Hoistables and Resources
([sebmarkbage](https://github.com/sebmarkbage) in
[#30584](#30584))
* chore[react-devtools]: add global for native and use it to fork
backend implementation ([hoxyq](https://github.com/hoxyq) in
[#30533](#30533))
* Enable pointEvents while scrolling
([sebmarkbage](https://github.com/sebmarkbage) in
[#30560](#30560))
* Make Element Inspection Feel Snappy
([sebmarkbage](https://github.com/sebmarkbage) in
[#30555](#30555))
* Track the parent DevToolsInstance while mounting a tree
([sebmarkbage](https://github.com/sebmarkbage) in
[#30542](#30542))
* Add DevToolsInstance to Store Stateful Information
([sebmarkbage](https://github.com/sebmarkbage) in
[#30517](#30517))
* Implement "best renderer" by taking the inner most matched node
([sebmarkbage](https://github.com/sebmarkbage) in
[#30494](#30494))
* Rename NativeElement to HostInstance in the Bridge
([sebmarkbage](https://github.com/sebmarkbage) in
[#30491](#30491))
* Rename Fiber to Element in the Bridge Protocol and RendererInterface
([sebmarkbage](https://github.com/sebmarkbage) in
[#30490](#30490))
* Stop filtering owner stacks
([sebmarkbage](https://github.com/sebmarkbage) in
[#30438](#30438))
* [Fiber] Call life-cycles with a react-stack-bottom-frame stack frame
([sebmarkbage](https://github.com/sebmarkbage) in
[#30429](#30429))
* [Flight] Prefix owner stacks added to the console.log with the current
stack ([sebmarkbage](https://github.com/sebmarkbage) in
[#30427](#30427))
* [BE] switch to hermes parser for prettier
([kassens](https://github.com/kassens) in
[#30421](#30421))
* Implement Owner Stacks ([sebmarkbage](https://github.com/sebmarkbage)
in [#30417](#30417))
* [BE] upgrade prettier to 3.3.3 ([kassens](https://github.com/kassens)
in [#30420](#30420))
* [ci] Add yarn_test_build job to gh actions
* [Fizz] Refactor Component Stack Nodes
([sebmarkbage](https://github.com/sebmarkbage) in
[#30298](#30298))
* Print component stacks as error objects to get source mapping
([sebmarkbage](https://github.com/sebmarkbage) in
[#30289](#30289))
* Upgrade flow to 0.235.0 ([kassens](https://github.com/kassens) in
[#30118](#30118))
* fix: path handling in react devtools
([Jack-Works](https://github.com/Jack-Works) in
[#29199](#29199))
hoxyq added a commit that referenced this pull request Sep 26, 2024
…ectorDataForViewAtPoint (#31068)

React DevTools no longer operates with just Fibers, it now builds its
own Shadow Tree, which represents the tree on the Host (Fabric on
Native, DOM on Web).

We have to keep track of public instances for a select-to-inspect
feature. We've recently changed this logic in
#30831, and looks like we've been
incorrectly getting a public instance for Fabric case.

Not only this, turns out that all `getInspectorData...` APIs are
returning Fibers, and not public instances. I have to expose it, so that
React DevTools can correctly identify the element, which was selected.

Changes for React Native are in
[D63421463](https://www.internalfb.com/diff/D63421463)
github-actions bot pushed a commit that referenced this pull request Sep 26, 2024
…ectorDataForViewAtPoint (#31068)

React DevTools no longer operates with just Fibers, it now builds its
own Shadow Tree, which represents the tree on the Host (Fabric on
Native, DOM on Web).

We have to keep track of public instances for a select-to-inspect
feature. We've recently changed this logic in
#30831, and looks like we've been
incorrectly getting a public instance for Fabric case.

Not only this, turns out that all `getInspectorData...` APIs are
returning Fibers, and not public instances. I have to expose it, so that
React DevTools can correctly identify the element, which was selected.

Changes for React Native are in
[D63421463](https://www.internalfb.com/diff/D63421463)

DiffTrain build for commit d66fa02.
hoxyq added a commit to hoxyq/react-native that referenced this pull request Sep 26, 2024
…ectorDataForViewAtPoint (facebook#31068)

Summary:
React DevTools no longer operates with just Fibers, it now builds its
own Shadow Tree, which represents the tree on the Host (Fabric on
Native, DOM on Web).

We have to keep track of public instances for a select-to-inspect
feature. We've recently changed this logic in
facebook/react#30831, and looks like we've been
incorrectly getting a public instance for Fabric case.

Not only this, turns out that all `getInspectorData...` APIs are
returning Fibers, and not public instances. I have to expose it, so that
React DevTools can correctly identify the element, which was selected.

Changes for React Native are in
[D63421463](https://www.internalfb.com/diff/D63421463)

DiffTrain build for commit facebook/react@d66fa02.

Differential Revision: D63453667
hoxyq added a commit to hoxyq/react-native that referenced this pull request Sep 26, 2024
…ectorDataForViewAtPoint (facebook#31068)

Summary:
React DevTools no longer operates with just Fibers, it now builds its
own Shadow Tree, which represents the tree on the Host (Fabric on
Native, DOM on Web).

We have to keep track of public instances for a select-to-inspect
feature. We've recently changed this logic in
facebook/react#30831, and looks like we've been
incorrectly getting a public instance for Fabric case.

Not only this, turns out that all `getInspectorData...` APIs are
returning Fibers, and not public instances. I have to expose it, so that
React DevTools can correctly identify the element, which was selected.

Changes for React Native are in
[D63421463](https://www.internalfb.com/diff/D63421463)

DiffTrain build for commit facebook/react@d66fa02.

Differential Revision: D63453667
hoxyq added a commit to hoxyq/react-native that referenced this pull request Sep 26, 2024
…ectorDataForViewAtPoint (facebook#31068)

Summary:
React DevTools no longer operates with just Fibers, it now builds its
own Shadow Tree, which represents the tree on the Host (Fabric on
Native, DOM on Web).

We have to keep track of public instances for a select-to-inspect
feature. We've recently changed this logic in
facebook/react#30831, and looks like we've been
incorrectly getting a public instance for Fabric case.

Not only this, turns out that all `getInspectorData...` APIs are
returning Fibers, and not public instances. I have to expose it, so that
React DevTools can correctly identify the element, which was selected.

Changes for React Native are in
[D63421463](https://www.internalfb.com/diff/D63421463)

DiffTrain build for commit facebook/react@d66fa02.

Reviewed By: poteto

Differential Revision: D63453667

Pulled By: hoxyq
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Sep 26, 2024
…ectorDataForViewAtPoint (#31068) (#46674)

Summary:
Pull Request resolved: #46674

bypass-github-export-checks

React DevTools no longer operates with just Fibers, it now builds its
own Shadow Tree, which represents the tree on the Host (Fabric on
Native, DOM on Web).

We have to keep track of public instances for a select-to-inspect
feature. We've recently changed this logic in
facebook/react#30831, and looks like we've been
incorrectly getting a public instance for Fabric case.

Not only this, turns out that all `getInspectorData...` APIs are
returning Fibers, and not public instances. I have to expose it, so that
React DevTools can correctly identify the element, which was selected.

Changes for React Native are in
[D63421463](https://www.internalfb.com/diff/D63421463)

DiffTrain build for commit facebook/react@d66fa02.

Test Plan: Sandcastle tests

Reviewed By: poteto

Differential Revision: D63453667

Pulled By: hoxyq

fbshipit-source-id: 21b1d5d4cd68b42748d4a785e0f28eaf5db57f21
mofeiZ added a commit that referenced this pull request Oct 11, 2024
This lets us get from a HostInstance to the nearest DevToolsInstance
without relying on `findFiberByHostInstance` and
`fiberToDevToolsInstanceMap`. We already did the equivalent of this for
Resources in HostHoistables.

One issue before was that we'd ideally get away from the
`fiberToDevToolsInstanceMap` map in general since we should ideally not
treat Fibers as stateful but they could be replaced by something else
stateful in principle.

This PR also addresses Virtual Instances. Now you can select a DOM node
and have it select a Virtual Instance if that's the nearest parent since
the parent doesn't have to be a Fiber anymore.

However, the other reason for this change is that I'd like to get rid of
the need for the `findFiberByHostInstance` from being injected. A
renderer should not need to store a reference back from its instance to
a Fiber. Without the Synthetic Event system this wouldn't be needed by
the renderer so we should be able to remove it. We also don't really
need it since we have all the information by just walking the commit to
collect the nodes if we just maintain our own Map.

There's one subtle nuance that the different renderers do. Typically a
HostInstance is the same thing as a PublicInstance in React but
technically in Fabric they're not the same. So we need to translate
between PublicInstance and HostInstance. I just hardcoded the Fabric
implementation of this since it's the only known one that does this but
could feature detect other ones too if necessary. On one hand it's more
resilient to refactors to not rely on injected helpers and on hand it
doesn't follow changes to things like this.

For the conflict resolution I added in #30494 I had to make that
specific to DOM so we can move the DOM traversal to the backend instead
of the injected helper.

[ghstack-poisoned]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants