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][State Tooltip] Added a stack of of component owners #24589

Closed
wants to merge 1 commit into from

Conversation

blakef
Copy link
Contributor

@blakef blakef commented May 20, 2022

Summary

demo.mp4

Added a side panel that:

  • list all parent components in the hierarchy of the Fiber initiating the state change
  • clickable link to inspect components in the hierarchy
  • list scrolls, ellipses for long component names

We can filter out built in components. Maybe as a follow up someone could add a checkbox that toggles this?

resolves #24170

How did you test this change?

Ran the change in the inline-devtools in the devtools-shell + as an extension in Chrome.

@lunaruan
Copy link
Contributor

Hey! This is a great first draft of the PR. 😍 It's great to see that you have an end to end implementation of this working!

A few comments/questions:

  1. I love the UI that you've created for the component stack! It conveys information in a clear and concise way. One thing I wonder if you've considered how the visualization of the component stack might be like in larger apps. For example, if you have a reusable button with many layers of parent components, it would be really hard, with only 5 elements in the component stack, to actually be able to identify where the button is used. What do you think about giving developers the ability to get all the elements in the stack instead of just the first 5? We don't have to visualize all of them (5 is probably a good number to start), but perhaps we can create some sort of UI to let the user get the rest of the components (perhaps a copy component stack button or a show more button?)
  2. Using the displayName is great in __DEV__ mode where the name (or user specified displayName) of the function is not minified. However, most people want to profile production apps (since dev apps tend to be a lot slower than production ones). In this case, often function names have been minified into a coupe of letters, which makes it really difficult for developers to debug. What do you think about adding the location of the function as well to make the function easier to find? There are a couple of strategies for this (checkout the function describeNativeComponentFrame). Feel free to ping me with questions!

@blakef
Copy link
Contributor Author

blakef commented May 20, 2022

I love these ideas. Let me come back to you with something.

@blakef blakef force-pushed the devtool-tooltip branch 4 times, most recently from a1323de to c7f9c4f Compare June 17, 2022 01:58
@blakef
Copy link
Contributor Author

blakef commented Jun 17, 2022

@lunaruan updated the video to show how it looks. There's plenty of room for small tweaks with the styling, and some of the UX (enabling / disabling built in components).

@@ -69,6 +69,7 @@ module.exports = {
new DefinePlugin({
__DEV__: true,
__PROFILE__: false,
__EXPERIMENTAL__: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was causing failures when trying to build devtools-extension for local on Chrome.

@blakef blakef force-pushed the devtool-tooltip branch 3 times, most recently from b42f6db to cacbe0e Compare June 17, 2022 11:53
@gaearon
Copy link
Collaborator

gaearon commented Jun 17, 2022

Should the stack be in the reverse order? So that you see the component itself at the top.

@lunaruan
Copy link
Contributor

Yayy! Glad you got the source working! 😊

One concern I have with this implementation is that you can only view the source when the fiber ID maps are accurate. These maps are only populated on render, which means that this implementation will only work if your'e profiling the active window and the fibers haven't been deleted from the map. Fibers are deleted from the map when they are removed from the fiber tree, and this is not an uncommon occurrence.

We also support uploading traces to React DevTools if you want Chrome performance data on top of your React Timeline data. We are working with Chrome on a better integrate method, but until then, you can only get screenshots, Chrome flamegraph chats, performance marks, etc. if you profile first in chrome and then upload to the React Profiler. This implementation will also not work in this case, because the IDs will be associated with the wrong fiber (or not present at all)

Going back up to my previous comment, I think jumping to source is a great idea if the source is available and it's possible to implement, but if it isn't, what do you think about storing the location instead of the parent ID and allowing for some sort of "go to editor" method instead?

You now get some idea of the context of a component that causes a state change,
with a sidebar listing the component's hierarchy. This additionally allows user
to click on components to link to their source code.

resolves facebook#24170
@blakef
Copy link
Contributor Author

blakef commented Jun 17, 2022

@lunaruan yea, sorry you did make this point earlier. I've still got that code lying around in the reflog.

@blakef
Copy link
Contributor Author

blakef commented Jun 28, 2022

Closing this in favour of #24805

@blakef blakef closed this Jun 28, 2022
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.

[React DevTools] Component Stacks for Timeline Profiler
4 participants