[DevTools] feat: show changed hooks names in the Profiler tab #31398
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Hey, thanks for upstreaming this, really appreciate any contributions to React DevTools. I am currently on leave and will be back first week of December. This change will probably require some manual testing on my side, so I think its better to iterate on this once I am fully available. |
| const [parseHookNamesOptimistic, setParseHookNamesOptimistic] = | ||
| useState(parseHookNames); | ||
|
|
||
| useEffect(() => { | ||
| setParseHookNamesOptimistic(parseHookNames); | ||
| }, [inspectedElement?.id, parseHookNames]); |
There was a problem hiding this comment.
This looks strange, you are keeping something from a global context in a local state, and then reconciliate it here? Why parseHookNames from InspectedElementContext is not enough?
There was a problem hiding this comment.
This approach is something I borrowed from the Inspected Element Panel here.
The idea behind the parseHookNamesOptimistic state is to keep the UI responsive. Since changing parseHookNames happens in a transition (which can suspend), this local state ensures the toggle feels immediate while the actual state updates in the background.
I considered making this logic reusable, by extracting LoadHookNamesButton into its own component, but I didn’t want to optimize prematurely. Do you think it’s worth revisiting?
There was a problem hiding this comment.
Oh, okay, we should probably refactor this to use useOptimistic in the future.
| const hookParsingFailed = useMemo( | ||
| () => parseHookNames && hookNames === null, | ||
| [parseHookNames, hookNames], | ||
| ); |
There was a problem hiding this comment.
You probably don't need useMemo for this one
packages/react-devtools-shared/src/devtools/views/Profiler/HookChangeSummary.js
Show resolved
Hide resolved
hoxyq
left a comment
hoxyq
left a comment
There was a problem hiding this comment.
Looks good overall, please address the comment that I've left.
I am planning to manually test this next week, and if everything works as expected, we can merge this
| return ( | ||
| hook.subHooks?.some(subHook => shouldKeepHook(subHook, hooksArray)) ?? false | ||
| ); |
There was a problem hiding this comment.
| return ( | |
| hook.subHooks?.some(subHook => shouldKeepHook(subHook, hooksArray)) ?? false | |
| ); | |
| const subHooks = hook.subHooks; | |
| if (subHooks == null) { | |
| return false; | |
| } | |
| return subHooks.some(subHook => shouldKeepHook(subHook, hooksArray)); |
| if (hook.subHooks?.length > 0) { | ||
| const filteredSubHooks = hook.subHooks | ||
| .map(subHook => filterHooks(subHook, hooksArray)) | ||
| .filter(Boolean); | ||
|
|
||
| return filteredSubHooks.length > 0 | ||
| ? {...hook, subHooks: filteredSubHooks} | ||
| : {...hook}; | ||
| } | ||
|
|
||
| return hook; |
There was a problem hiding this comment.
| if (hook.subHooks?.length > 0) { | |
| const filteredSubHooks = hook.subHooks | |
| .map(subHook => filterHooks(subHook, hooksArray)) | |
| .filter(Boolean); | |
| return filteredSubHooks.length > 0 | |
| ? {...hook, subHooks: filteredSubHooks} | |
| : {...hook}; | |
| } | |
| return hook; | |
| const subHooks = hook.subHooks; | |
| if (subHooks == null) { | |
| return hook; | |
| } | |
| const filteredSubHooks = subHooks | |
| .map(subHook => filterHooks(subHook, hooksArray)) | |
| .filter(Boolean); | |
| return filteredSubHooks.length > 0 | |
| ? {...hook, subHooks: filteredSubHooks} | |
| : hook; |
| </span> | ||
| {hook.subHooks?.map((subHook, index) => ( | ||
| <Hook | ||
| key={`${hook.id}-${index}`} |
There was a problem hiding this comment.
Do you really need an index here for constructing a key? hook.id and index actually have a correlation, because both of them depend on the fact when the hook was defined relatively to other hooks: in the end all of them are going to be stored in a linked list.
There was a problem hiding this comment.
I remember it was a dirty fix to prevent stale hook info, but I couldn't reproduce it now, so I removed it.
|
Can this be moved forward ? |
|
@piotrski Are you planning to continue the work on this or are you okay with someone picking this up? |
|
@hoxyq sorry, I was sick, and then I got caught up with other things and completely missed this. I’ll try to get it done this week since I finally have some time for OSS |
No worries, thank you! |
…cache The InspectedElementContext was previously only available in the Components tab. This change moves the context provider to a higher level in the component hierarchy, making the InspectedElementContext available in both the Components and Profiler tabs.
- Introduced `HookChangeSummary` component to show names of changed hooks, improving clarity and making it easier to identify specific hook updates in the Profiler tab. - Added `getAlreadyLoadedHookNames` helper for efficient retrieval of hook names. Resolves facebook#21856
…ry in tooltips - Added `displayMode` prop to `HookChangeSummary` for `detailed` and `compact` views. - Updated `WhatChanged` and `HoveredFiberInfo` to use `compact` mode in tooltips, preserving the previous concise summary display. This change maintains the original hook change summary style in tooltips while allowing a detailed view where needed.
piotrski
force-pushed
the
profiler-hook-names
branch
from
67f7b6e to
ed0f79d
Compare
piotrski
force-pushed
the
profiler-hook-names
branch
from
ed0f79d to
e24324f
Compare
|
@hoxyq I’ve addressed all the comments, let me know if anything else needs further adjustments. Looking forward to your feedback. |
Makes sense, that would be awesome.
Yeah, the implementation is flawed, and was based on a lot of assumptions about the source code, but that's what we have. This should not block you from landing this change, since you are just using it as an API. |
hoxyq
left a comment
hoxyq
left a comment
There was a problem hiding this comment.
Looks good, thanks for working on this! I will run few manual tests later this week and merge it if everything is alright.
|
Hey, just circling back here to say that this is still in my queue. In my scenario, I end up in a weird state where I can see the button for fetching hook names on the inspected element panel, but not on the profile tab, I am only seeing hooks indexes. We should probably avoid doing this. I am not sure yet what is going wrong, though. Will follow up here once I have more details. |
|
@hoxyq thanks for the update! Happy to jump on a quick call if that helps speed up debugging, just let me know. |
hoxyq
left a comment
hoxyq
left a comment
There was a problem hiding this comment.
All good with changes in this diff, thanks for contributing. I am going to proceed with merging this.
Before the next release, I will try to find some time to fix the hook names parsing implementation, because right now it is mostly broken for Flow and extremely slow for larger apps. I don't have an approximal date for next release yet.
|
Thanks for merging! Yeah, I’ve noticed the hook name parsing can be a bit flaky too, especially in Next.js projects. If there's anything I can do to help with that, just let me know. I'm happy to help. |
5 participants
Summary
This PR adds support for displaying the names of changed hooks directly in the Profiler tab, making it easier to identify specific updates.
A
HookChangeSummarycomponent has been introduced to show these hook names, with adisplayModeprop that toggles between“compact”for tooltips and“detailed”for more in-depth views. This keeps tooltip summaries concise while allowing for a full breakdown where needed.This functionality also respects the
“Always parse hook names from source”setting from the Component inspector, as it uses the same caching mechanism already in place for the Components tab. Additionally, even without hook names parsed, the Profiler will now display hook types (likeState,Callback, etc.) based on data frominspectedElement.To enable this across the DevTools,
InspectedElementContexthas been moved higher in the component tree, allowing it to be shared between the Profiler and Components tabs. This update allows hook name data to be reused across tabs without duplication.Additionally, a
getAlreadyLoadedHookNameshelper function was added to efficiently access cached hook names, reducing the need for repeated fetching when displaying changes.These changes improve the ability to track specific hook updates within the Profiler tab, making it clearer to see what’s changed.
Before
Previously, the Profiler tab displayed only the IDs of changed hooks, as shown below:
After (without hook names parsed)
When hook names aren’t parsed, custom hooks and hook types are displayed based on the inspectedElement data:
After (with hook names parsed)
Once hook names are fully parsed, the Profiler tab provides a complete breakdown of specific hooks that have changed:
This should resolve #21856 🎉