-
Notifications
You must be signed in to change notification settings - Fork 50.2k
[DevTools] Fix hooks indexes counting #34427
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
base: main
Are you sure you want to change the base?
[DevTools] Fix hooks indexes counting #34427
Conversation
|
Comparing: 8943025...0e9630b Critical size changesIncludes critical production bundles, as well as any change greater than 2%: Significant size changesIncludes any change greater than 0.2%: Expand to show
|
0e9630b to
0f6b299
Compare
|
I tried running |
0f6b299 to
e75a779
Compare
884fb92 to
e606c53
Compare
|
I fixed the unit tests too, but not sure if this is the right solution or if the
@bvaughn @lunaruan @acdlite @eps1lon @rickhanlonii What do you think about this bug fix? I think it is important to fix this, as it helped me a lot as a React learning tool. |
|
This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated. |
|
bump |
e606c53 to
859f91a
Compare
…ore (facebook#34547)" This reverts commit 39c6545.
Summary
Fixed hooks indexes counting to reflect the ones shown in
Why did this render? Hook <index> changed. The problem is that the index logic at https://github.com/facebook/react/blob/main/packages/react-devtools-shared/src/backend/fiber/renderer.js#L1951 (used byWhy did this render?messages) counts all nodes in the hooks linked list of a fiber, but the index logic based on this array https://github.com/facebook/react/blob/main/packages/react-debug-tools/src/ReactDebugHooks.js#L54 (used by indexes shown in Component hooks inspector) does not push an element in thehookLogarray for each node added to the hooks linked list of a fiber for some composite React hooks:useTransitionuseSyncExternalStateuseFormStateuseActionStateThis is the reason why, when profiling re-renders for Components having
useSyncExternalStateinside them (coming fromzustand, for example), the indexes shown inWhy did this render?messages do not match the indexes shown by the Component hooks inspector.The changes in this PR add elements to the
hookLogarray corresponding to the hook linked list nodes created by the 4 composite hooks above.This bug exists probably because the changed hook indexes were added when composite hooks did not exist yet, apart from
useTransition:How did you test this change?
I needed to fix this problem in order to debug re-render issues in a React Native app. I tested this fix by compiling the React Native app with a custom
react-nativebuild which uses a customreact-devtools-corebuild based on this branch.