Skip to content

Commit 53daaf5

Browse files
blazejkustrahoxyq
andauthored
Improve the detection of changed hooks (facebook#35123)
## Summary cc @hoxyq Fixes facebook#28584. Follow up to PR: facebook#34547 This PR updates getChangedHooksIndices to account for the fact that `useSyncExternalStore`, `useTransition`, `useActionState`, `useFormState` internally mounts more than one hook while DevTools should treat it as a single user-facing hook. Approach idea came from [this](facebook#34547 (comment)) comment 😄 Before: https://github.com/user-attachments/assets/6bd5ce80-8b52-4bb8-8bb1-5e91b1e65043 After: https://github.com/user-attachments/assets/47f56898-ab34-46b6-be7a-a54024dcefee ## How did you test this change? I used this component to reproduce this issue locally (I followed instructions in `packages/react-devtools/CONTRIBUTING.md`). <details><summary>Details</summary> ```ts import * as React from 'react'; function useDeepNestedHook() { React.useState(0); // 1 return React.useState(1); // 2 } function useNestedHook() { const deepState = useDeepNestedHook(); React.useState(2); // 3 React.useState(3); // 4 return deepState; } // Create a simple store for useSyncExternalStore function createStore(initialValue) { let value = initialValue; const listeners = new Set(); return { getSnapshot: () => value, subscribe: listener => { listeners.add(listener); return () => { listeners.delete(listener); }; }, update: newValue => { value = newValue; listeners.forEach(listener => listener()); }, }; } const syncExternalStore = createStore(0); export default function InspectableElements(): React.Node { const [nestedState, setNestedState] = useNestedHook(); // 5 const syncExternalValue = React.useSyncExternalStore( syncExternalStore.subscribe, syncExternalStore.getSnapshot, ); // 6 const [isPending, startTransition] = React.useTransition(); // 7 const [formState, formAction, formPending] = React.useActionState( async (prevState, formData) => { return {count: (prevState?.count || 0) + 1}; }, {count: 0}, ); const handleTransition = () => { startTransition(() => { setState(Math.random()); }); }; // 8 const [state, setState] = React.useState('test'); return ( <> <div style={{ padding: '20px', display: 'flex', flexDirection: 'column', gap: '10px', }}> <div onClick={() => setNestedState(Math.random())} style={{backgroundColor: 'red', padding: '10px', cursor: 'pointer'}}> State: {nestedState} </div> <button onClick={handleTransition} style={{padding: '10px'}}> Trigger Transition {isPending ? '(pending...)' : ''} </button> <div style={{display: 'flex', gap: '10px', alignItems: 'center'}}> <button onClick={() => syncExternalStore.update(syncExternalValue + 1)} style={{padding: '10px'}}> Trigger useSyncExternalStore </button> <span>Value: {syncExternalValue}</span> </div> <form action={formAction} style={{display: 'flex', gap: '10px', alignItems: 'center'}}> <button type="submit" style={{padding: '10px'}} disabled={formPending}> Trigger useFormState {formPending ? '(pending...)' : ''} </button> <span>Count: {formState.count}</span> </form> <div onClick={() => setState(Math.random())} style={{backgroundColor: 'red', padding: '10px', cursor: 'pointer'}}> State: {state} </div> </div> </> ); } ``` </details> --------- Co-authored-by: Ruslan Lesiutin <28902667+hoxyq@users.noreply.github.com>
1 parent 4a3d993 commit 53daaf5

File tree

4 files changed

+290
-68
lines changed

4 files changed

+290
-68
lines changed

packages/react-debug-tools/src/ReactDebugHooks.js

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -467,9 +467,11 @@ function useSyncExternalStore<T>(
467467
// useSyncExternalStore() composes multiple hooks internally.
468468
// Advance the current hook index the same number of times
469469
// so that subsequent hooks have the right memoized state.
470-
nextHook(); // SyncExternalStore
470+
const hook = nextHook(); // SyncExternalStore
471471
nextHook(); // Effect
472-
const value = getSnapshot();
472+
// Read from hook.memoizedState to get the value that was used during render,
473+
// not the current value from getSnapshot() which may have changed.
474+
const value = hook !== null ? hook.memoizedState : getSnapshot();
473475
hookLog.push({
474476
displayName: null,
475477
primitive: 'SyncExternalStore',

packages/react-devtools-shared/src/__tests__/profilerChangeDescriptions-test.js

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ describe('Profiler change descriptions', () => {
9090
{
9191
"context": true,
9292
"didHooksChange": false,
93-
"hooks": null,
93+
"hooks": [],
9494
"isFirstMount": false,
9595
"props": [],
9696
"state": null,
@@ -110,7 +110,7 @@ describe('Profiler change descriptions', () => {
110110
{
111111
"context": true,
112112
"didHooksChange": false,
113-
"hooks": null,
113+
"hooks": [],
114114
"isFirstMount": false,
115115
"props": [],
116116
"state": null,
@@ -125,7 +125,7 @@ describe('Profiler change descriptions', () => {
125125
{
126126
"context": false,
127127
"didHooksChange": false,
128-
"hooks": null,
128+
"hooks": [],
129129
"isFirstMount": false,
130130
"props": [],
131131
"state": null,
@@ -140,7 +140,7 @@ describe('Profiler change descriptions', () => {
140140
{
141141
"context": true,
142142
"didHooksChange": false,
143-
"hooks": null,
143+
"hooks": [],
144144
"isFirstMount": false,
145145
"props": [],
146146
"state": null,

packages/react-devtools-shared/src/__tests__/profilingCache-test.js

Lines changed: 246 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -378,6 +378,12 @@ describe('ProfilingCache', () => {
378378
),
379379
);
380380

381+
// Save references to the real dispatch/setState functions.
382+
// inspectHooks() re-runs the component with a mock dispatcher,
383+
// which would overwrite these variables with mock functions that do nothing.
384+
const realDispatch = dispatch;
385+
const realSetState = setState;
386+
381387
// Second render has no changed hooks, only changed props.
382388
utils.act(() =>
383389
render(
@@ -388,10 +394,10 @@ describe('ProfilingCache', () => {
388394
);
389395

390396
// Third render has a changed reducer hook.
391-
utils.act(() => dispatch({type: 'invert'}));
397+
utils.act(() => realDispatch({type: 'invert'}));
392398

393399
// Fourth render has a changed state hook.
394-
utils.act(() => setState('def'));
400+
utils.act(() => realSetState('def'));
395401

396402
// Fifth render has a changed context value, but no changed hook.
397403
utils.act(() =>
@@ -521,6 +527,238 @@ describe('ProfilingCache', () => {
521527
}
522528
});
523529

530+
// @reactVersion >= 19.0
531+
it('should detect what hooks changed in a render with custom and composite hooks', () => {
532+
let snapshot = 0;
533+
let syncExternalStoreCallback;
534+
535+
function subscribe(callback) {
536+
syncExternalStoreCallback = callback;
537+
return () => {};
538+
}
539+
540+
function getSnapshot() {
541+
return snapshot;
542+
}
543+
544+
// Custom hook wrapping multiple primitive hooks
545+
function useCustomHook() {
546+
const [value, setValue] = React.useState('custom');
547+
React.useEffect(() => {}, [value]);
548+
return [value, setValue];
549+
}
550+
551+
let setState = null;
552+
let startTransition = null;
553+
let actionStateDispatch = null;
554+
let setCustomValue = null;
555+
let setFinalState = null;
556+
557+
const Component = () => {
558+
// Hook 0: useState
559+
const [state, _setState] = React.useState('initial');
560+
setState = _setState;
561+
562+
// Hook 1: useSyncExternalStore (composite hook - internally uses multiple hooks)
563+
const storeValue = React.useSyncExternalStore(
564+
subscribe,
565+
getSnapshot,
566+
getSnapshot,
567+
);
568+
569+
// Hook 2: useTransition (composite hook - internally uses multiple hooks)
570+
const [isPending, _startTransition] = React.useTransition();
571+
startTransition = _startTransition;
572+
573+
// Hook 3: useActionState (composite hook - internally uses multiple hooks)
574+
const [actionState, _actionStateDispatch] = React.useActionState(
575+
(_prev, action) => action,
576+
'action-initial',
577+
);
578+
actionStateDispatch = _actionStateDispatch;
579+
580+
// Hook 4: useState inside custom hook (flattened)
581+
// Hook 5: useEffect inside custom hook (not stateful, won't show in changes)
582+
const [customValue, _setCustomValue] = useCustomHook();
583+
setCustomValue = _setCustomValue;
584+
585+
// Hook 6: direct useState at the end
586+
const [finalState, _setFinalState] = React.useState('final');
587+
setFinalState = _setFinalState;
588+
589+
return `${state}-${storeValue}-${isPending}-${actionState}-${customValue}-${finalState}`;
590+
};
591+
592+
utils.act(() => store.profilerStore.startProfiling());
593+
utils.act(() => render(<Component />));
594+
595+
// Save references before inspectHooks() overwrites them
596+
const realSetState = setState;
597+
const realStartTransition = startTransition;
598+
const realActionStateDispatch = actionStateDispatch;
599+
const realSetCustomValue = setCustomValue;
600+
const realSetFinalState = setFinalState;
601+
602+
// 2nd render: change useState (hook 0)
603+
utils.act(() => realSetState('changed'));
604+
605+
// 3rd render: change useSyncExternalStore (hook 1)
606+
utils.act(() => {
607+
snapshot = 1;
608+
syncExternalStoreCallback();
609+
});
610+
611+
// 4th render: trigger useTransition (hook 2)
612+
// Note: useTransition triggers two renders - one when isPending becomes true,
613+
// and another when isPending becomes false after the transition completes
614+
utils.act(() => {
615+
realStartTransition(() => {});
616+
});
617+
618+
// 6th render: change useActionState (hook 3)
619+
utils.act(() => realActionStateDispatch('action-changed'));
620+
621+
// 7th render: change custom hook's useState (hook 4)
622+
utils.act(() => realSetCustomValue('custom-changed'));
623+
624+
// 8th render: change final useState (hook 6)
625+
utils.act(() => realSetFinalState('final-changed'));
626+
627+
utils.act(() => store.profilerStore.stopProfiling());
628+
629+
const rootID = store.roots[0];
630+
631+
const changeDescriptions = store.profilerStore
632+
.getDataForRoot(rootID)
633+
.commitData.map(commitData => commitData.changeDescriptions);
634+
expect(changeDescriptions).toHaveLength(8);
635+
636+
// 1st render: Initial mount
637+
expect(changeDescriptions[0]).toMatchInlineSnapshot(`
638+
Map {
639+
2 => {
640+
"context": null,
641+
"didHooksChange": false,
642+
"isFirstMount": true,
643+
"props": null,
644+
"state": null,
645+
},
646+
}
647+
`);
648+
649+
// 2nd render: Changed hook 0 (useState)
650+
expect(changeDescriptions[1]).toMatchInlineSnapshot(`
651+
Map {
652+
2 => {
653+
"context": false,
654+
"didHooksChange": true,
655+
"hooks": [
656+
0,
657+
],
658+
"isFirstMount": false,
659+
"props": [],
660+
"state": null,
661+
},
662+
}
663+
`);
664+
665+
// 3rd render: Changed hook 1 (useSyncExternalStore)
666+
expect(changeDescriptions[2]).toMatchInlineSnapshot(`
667+
Map {
668+
2 => {
669+
"context": false,
670+
"didHooksChange": true,
671+
"hooks": [
672+
1,
673+
],
674+
"isFirstMount": false,
675+
"props": [],
676+
"state": null,
677+
},
678+
}
679+
`);
680+
681+
// 4th render: Changed hook 2 (useTransition - isPending becomes true)
682+
expect(changeDescriptions[3]).toMatchInlineSnapshot(`
683+
Map {
684+
2 => {
685+
"context": false,
686+
"didHooksChange": true,
687+
"hooks": [
688+
2,
689+
],
690+
"isFirstMount": false,
691+
"props": [],
692+
"state": null,
693+
},
694+
}
695+
`);
696+
697+
// 5th render: Changed hook 2 (useTransition - isPending becomes false)
698+
expect(changeDescriptions[4]).toMatchInlineSnapshot(`
699+
Map {
700+
2 => {
701+
"context": false,
702+
"didHooksChange": true,
703+
"hooks": [
704+
2,
705+
],
706+
"isFirstMount": false,
707+
"props": [],
708+
"state": null,
709+
},
710+
}
711+
`);
712+
713+
// 6th render: Changed hook 3 (useActionState)
714+
expect(changeDescriptions[5]).toMatchInlineSnapshot(`
715+
Map {
716+
2 => {
717+
"context": false,
718+
"didHooksChange": true,
719+
"hooks": [
720+
3,
721+
],
722+
"isFirstMount": false,
723+
"props": [],
724+
"state": null,
725+
},
726+
}
727+
`);
728+
729+
// 7th render: Changed hook 4 (useState inside useCustomHook)
730+
expect(changeDescriptions[6]).toMatchInlineSnapshot(`
731+
Map {
732+
2 => {
733+
"context": false,
734+
"didHooksChange": true,
735+
"hooks": [
736+
4,
737+
],
738+
"isFirstMount": false,
739+
"props": [],
740+
"state": null,
741+
},
742+
}
743+
`);
744+
745+
// 8th render: Changed hook 6 (final useState)
746+
expect(changeDescriptions[7]).toMatchInlineSnapshot(`
747+
Map {
748+
2 => {
749+
"context": false,
750+
"didHooksChange": true,
751+
"hooks": [
752+
6,
753+
],
754+
"isFirstMount": false,
755+
"props": [],
756+
"state": null,
757+
},
758+
}
759+
`);
760+
});
761+
524762
// @reactVersion >= 19.0
525763
it('should detect context changes or lack of changes with conditional use()', () => {
526764
const ContextA = React.createContext(0);
@@ -553,6 +791,11 @@ describe('ProfilingCache', () => {
553791
),
554792
);
555793

794+
// Save reference to the real setState function before profiling starts.
795+
// inspectHooks() re-runs the component with a mock dispatcher,
796+
// which would overwrite setState with a mock function that does nothing.
797+
const realSetState = setState;
798+
556799
utils.act(() => store.profilerStore.startProfiling());
557800

558801
// First render changes Context.
@@ -567,7 +810,7 @@ describe('ProfilingCache', () => {
567810
);
568811

569812
// Second render has no changed Context, only changed state.
570-
utils.act(() => setState('def'));
813+
utils.act(() => realSetState('def'));
571814

572815
utils.act(() => store.profilerStore.stopProfiling());
573816

0 commit comments

Comments
 (0)