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.

Sign up for GitHub

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

Jump to bottom

[DevTools] Implement "best renderer" by taking the inner most matched node #30494

Merged
merged 1 commit into from
Jul 30, 2024
Merged

[DevTools] Implement "best renderer" by taking the inner most matched node #30494

merged 1 commit into from
Jul 30, 2024

Conversation

sebmarkbage
Copy link
Collaborator

Stacked on #30491.

When going from DOM Node to select a component or highlight a component we find the nearest mounted ancestor. However, when multiple renderers are nested there can be multiple ancestors. The original fix #24665 did this by taking the inner renderer if it was an exact match but if it wasn't it just took the first renderer.

Instead, we can track the inner most node we've found so far. Then get the ID from that node (which will be fast since it's now a perfect match). This is a better match.

However, the main reason I'm doing this is because the old mechanism leaked the Fiber type outside the RendererInterface which is supposed to abstract all of that. With the new algorithm this doesn't leak.

I've tested this with a new build against the repro in the old issue #24539 and it seems to work.

@sebmarkbage sebmarkbage requested a review from hoxyq July 28, 2024 20:15
@vercel Vercel
Copy link

vercel bot commented Jul 28, 2024 •
edited
Loading

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 Jul 30, 2024 1:15pm

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jul 28, 2024
hoxyq
hoxyq approved these changes Jul 29, 2024
View reviewed changes
packages/react-devtools-shared/src/backend/agent.js
const rendererInterface = this.getBestMatchingRendererInterface(node);
if (rendererInterface != null) {
getComponentNameForHostInstance(target: HostInstance): string | null {
// We duplicate this code from getIDForHostInstance to avoid an object allocation.
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean by that? Which object is going to be allocated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Like if you try to extract this into a helper you end up with the problem that JS can't return two values. It'd need to return the ID and the renderer. So you'd have to wrap that in an object and that would end up being an unnecessary allocation.

@sebmarkbage sebmarkbage mentioned this pull request Jul 29, 2024
Merged
@sebmarkbage
Implement "best renderer" by taking the inner most matched node
f800bef 
Don't leak the "Fiber" type outside the renderer interface.
@sebmarkbage sebmarkbage merged commit f963c80 into facebook:main Jul 30, 2024
185 checks passed
sebmarkbage added a commit that referenced this pull request Jul 30, 2024
@sebmarkbage
[DevTools] Add DevToolsInstance to Store Stateful Information (#30517)
f827c79 
Stacked on #30494 and #30491.

This is setting us up to be able to track Server Components. This is now
split into a FiberInstance (Client Component) and a VirtualInstance
(Server Component). We're not actually creating any VirtualInstances yet
though this is just the data structures.

Server Components and potentially other compiled away or runtime
optimized away (e.g. calling through a function without creating an
intermediate Fiber) don't have a stateful instance. They just represent
the latest data. It's kind of like a React Element.

However, in the DevTools UI we need them to be stateful partly just so
that you can select and refer to them separately. E.g. the same Server
Component output rendered into different slots on the client should
still have two different representations in the DevTools. Also if the
same child Fibers update in place because the Server Component refreshed
we shouldn't lose the selection if you've selected a Server Component.

I'll implement this by creating Virtual Instances that only exist for
the purpose of the DevTools UI and so it'll be implemented in the
DevTools.

We could just make a Map from `id` to `Fiber | ReactComponentInfo` but
that requires a branching without a consistent hidden class. We also
need some more states on there. We also have some other Maps that tracks
extra states like that of component stacks, errors and warnings.
Constantly resizing and adding/removing from a Map isn't exactly fast.
It's faster to have a single Map with an object in it than one Map per
object. However, having extra fields that are usually just `null` can
instead mean more memory gets used. Since only a very small fraction of
instances will have errors/warnings or having initialized its component
stack, it made sense to store that in a separate Map that is usually
just empty.

However, with the addition of particularly the `parent` field and the
ability to do a fast hidden-class safe branching on the `kind` field I
think it's probably worth actually allocating an extra first class
object per Instance to store DevTools state into. That's why I converted
from just storing `Fiber` -> `id` to storing `Fiber` ->
`DevToolsInstance` which then keeps the warnings/errors/componentStack
as extra fields that are usually `null`. That is a lot of objects though
since it's one per Fiber pair basically.
@eps1lon eps1lon mentioned this pull request Jul 30, 2024
Merged
@sebmarkbage sebmarkbage mentioned this pull request Aug 28, 2024
Merged
sebmarkbage added a commit that referenced this pull request Sep 3, 2024
@sebmarkbage
[DevTools] Track all public HostInstances in a Map (#30831)
d1afcb4 
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.
@hoxyq hoxyq mentioned this pull request Sep 25, 2024
Merged
hoxyq added a commit that referenced this pull request Sep 25, 2024
@hoxyq
React DevTools 5.3.1 -> 6.0.0 (#31058)
d2e9b9b 
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))
mofeiZ added a commit that referenced this pull request Oct 11, 2024
@mofeiZ
[DevTools] Track all public HostInstances in a Map (#30831)
e2c0670 
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]
@mofeiZ mofeiZ mentioned this pull request Oct 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hoxyq hoxyq hoxyq approved these changes

Assignees
No one assigned
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sebmarkbage @hoxyq @facebook-github-bot