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: Profiler: Show which hooks changed #16477

Open
bvaughn opened this issue Aug 19, 2019 · 17 comments
Open

DevTools: Profiler: Show which hooks changed #16477

bvaughn opened this issue Aug 19, 2019 · 17 comments
Labels

Comments

@bvaughn
Copy link
Contributor

bvaughn commented Aug 19, 2019

"Can you show which hooks changed?"

...is a question I've heard a couple of times with regard to the new Profiler change-tracking feature. This request is certainly understandable, but it presents a couple of challenges:

  1. Identifying which hooks values change would requires shallowly re-rendering each function component.
  2. Identifying a hook in a non-ambiguous way requires displaying the full hooks tree structure, since hooks aren't named. (Alternately we could support named hooks, Named hooks: MVP support #16474)

Let's take each of a look at each of these below.

1 - Identifying which hooks values change

One of the challenge for DevTools when it comes to hooks is identifying custom hooks. Sebastian's proposed solution is that DevTools temporarily overrides React's hooks dispatcher while it shallowly re-renders the component. During the re-render, each time one of the built-in hooks is used, our override implementation parses the stack to identify "custom hooks" (functions higher up in the callstack that begin with "use"). After render is completed, we reassemble this information into a tree structure which DevTools can display.

Currently we only do this shallow render when a component is inspected, but in order for us to track which hooks have changed while profiling, we would need to shallowly render every component using hooks during the profiling session. Mostly likely we would have to do this during the performance sensitive "commit" phase since that's when DevTools is notified of an update.

I think we could do better than re-running the above hooks override for every component on every commit if we:

  • Created a map of Fiber to cached hooks tree structure.
  • Lazily populate the above map (by shallow re-rendering) only when a component was updated for the first time.
  • Compared Fiber memoizedStates to identify changes on future commits and map them back to the tree structure based on their position in the list structure. 1

However, even with the above optimizations this would still add significant overhead to a performance sensitive phase.

1 I think this should work but might also end up being complicated to implement.

2 - Identifying a hook

Although the variables that hooks values are assigned to are meaningfully named, the hooks themselves are unnamed. Because of this, DevTools has no feasible way of identifying a hook short of displaying the entire hooks tree structure. Consider the following example code:

function useCustomHook(...) {
  const [foo, setFoo] = useState(...);
  // ...
}

function ExampleComponent(props) {
  const [bar, setBar] = useState(...);
  const [baz, setBaz] = useState(...);
  const custom = useCustomHook(...);
  // ...
}

The example above shows 4 hooks: three useState and one custom. Let's say that "foo" and "baz" changed in a particular render. How would DevTools identify this? It could just show "two state hooks" but that's not very helpful. I think the only way we could identify it would be to show the entire tree, and visually highlight which hooks in it have changed:

State
State *
CustomHook
  State *

This is okay but it's not great unless the developer is cross-referencing the component (and probably the custom hooks definition as well). To help with this, we could also show the values but now we're adding more overhead in terms of trackin and bridge traffic.

In summary

Clearly both of these challenges can be overcome but they are non-trivial to implement and they will certainly add more runtime overhead to the profiler. Because of this, it may be a while before we add this feature to the DevTools.


Originally reported via bvaughn/react-devtools-experimental#312

@evbo
Copy link

evbo commented Aug 29, 2019

@bvaughn so for a component that stores state only in useState hooks (and with no other hooks implemented), when the profile reports Hooks changed, is that synonymous with state changed?

When it reports specific state changing, is that only supported for class-based or ref state variables?

Thanks

@bvaughn
Copy link
Contributor Author

bvaughn commented Aug 29, 2019

Class components may have props, state, and/or context.

Function components may have props and/or hooks.

@bvaughn
Copy link
Contributor Author

bvaughn commented Sep 20, 2019

One possibility that wasn't listed in the original issue description is to only collect the indices of which hooks changed while profiling, and wait until inspection to lazily build up the profiling tree metadata. This would avoid the need to eagerly shallow render every function component with hooks to determine which hooks are being used, but it has the following downsides:

  • It wouldn't work for components that are unmounted before the profiling stops.
  • It wouldn't work at all for exported+imported Profiling data.

@justingrant
Copy link
Contributor

justingrant commented Dec 4, 2019

FWIW, identifying which hook changed is my top request for react devtools. Here's another reason why: I just spent three days (!!!) tracking down a problem that could have been identified in 5 minutes if dev tools could have shown which hooks changed during a render. I thought I'd found a browser bug (and wasted days chasing it!) when in fact it was just a deeply-nested component being unexpectedly re-rendered too often by a changed hook. ;-(

I understand wanting a works-in-prod-too solution, but even if the feature was dev-mode-only or was worse in prod, that'd be vastly better than the current state of affairs where hooks changing is a black box.

In order to find the bug I finally ended up writing a "log if this value changed" hook and called it for every hook return value across 20+ components. This was a huge hassle. Here's the hook in case anyone wants to copy:

// eslint-disable-next-line @typescript-eslint/no-explicit-any
function print(value: any) {
  if (typeof value === 'object') {
    return JSON.stringify(value);
  } else {
    return value.toString();
  }
}
function useLogIfChanged<T>(name: string, value: T) {
  const previous = useRef(value);
  if (!Object.is(previous.current, value)) {
    console.log(`${name} changed. Old: ${print(previous.current)}, New: ${print(value)} `);
    previous.current = value;
  }
}

@bertho-zero
Copy link

@justingrant https://github.com/welldone-software/why-did-you-render can save your life

@necolas necolas added the React Core Team Opened by a member of the React Core Team label Jan 8, 2020
@LukePetruzzi
Copy link

Really craving this feature right now attempting to diagnose rerenders in a large component - is this currently prioritized?

@william0bra

This comment has been minimized.

@Pringels
Copy link

I would also love this feature. As a workaround I did the following:

1.) Open react devtools
2.) Select the relevant component
3.) Copy the current hooks to clipboard
4.) Paste into text editor
5.) Trigger problematic re-render scenario (whatever action or activity is causing the re-render)
6.) Copy the hooks to clipboard again
7.) Paste into new window in text editor
8.) Use some difftool to compare

image

Granted this is a very laborious process, but it has served me quite well to diagnose hook-related re-renders. (especially when they are caused by deeply nested hooks, EG. apollo-client)

Hope this helps until a better solution lands :)

@jamesthomsondev
Copy link

+1, this would be a real game changer for the Profiler

@charlie0077
Copy link

+1 on this one. this would be huge!

I started missing the old days of using redux connector, it was easier to debug performance issues, compared to use hooks.

@bvaughn
Copy link
Contributor Author

bvaughn commented May 19, 2020

+1 comments aren't hat helpful folks. They just add noise to the issue 😄

We know the feature would be useful. It's kind of blocked on #16474 which doesn't have an obvious solution.

@burtonator
Copy link

I will name my first born male 'React' if you guys implement this feature!

@GriffinSauce
Copy link

A thought on meaningful names: I personally don't mind if the output is like the example from point 2. Knowing that all hooks are executed in order makes it trivial to find the offending hook.

I'd much rather have that solution earlier than a more "elegant" solution later.

@nibblesnbits
Copy link

Is there any movement on this? I would give an arm and a leg for a solution to this; even an inelegant one.

@bvaughn
Copy link
Contributor Author

bvaughn commented Mar 9, 2021

We will update the issue when there's an update to share. As I mentioned a few comments above:

We know the feature would be useful. It's kind of blocked on #16474 which doesn't have an obvious solution.

I'm going to lock the issue for now to limit the noise.

We know it's a useful addition. We're bandwidth constrained right now.

@facebook facebook locked and limited conversation to collaborators Mar 9, 2021
@bvaughn
Copy link
Contributor Author

bvaughn commented Mar 25, 2021

PR #20998 is related to this issue, though the approach we're trying is very clunky and probably wouldn't make sense to release more widely.

@bvaughn
Copy link
Contributor Author

bvaughn commented Jul 17, 2021

Note that a combination of #20998 and #21856 will eventually enable this feature to work the way everyone wants it to.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests