Skip to content

Commit

Permalink
[DevTools] Handle reordered contexts in Profiler (#30887)
Browse files Browse the repository at this point in the history
While looking at the Context tracking implementation for other reasons I
noticed this bug.

Originally it wasn't allowed to have conditional `useContext(context)`
(although we did because it's technically possible). With `use(context)`
it is officially allowed to be conditional as long as it is within a
Hook/Component and not within a try/catch.

This means that this loop comparing previous and next contexts need to
consider that the Context objects might not line up and so it's possibly
comparing apples to oranges. We already bailed if one was longer than
the other.

If the order of contexts changes later in the component that means
something else must have already changed earlier so the reason for the
rerender isn't the context so we can just return false in that case.
  • Loading branch information
sebmarkbage authored Sep 7, 2024
1 parent 727b361 commit d76a565
Show file tree
Hide file tree
Showing 2 changed files with 108 additions and 0 deletions.
100 changes: 100 additions & 0 deletions packages/react-devtools-shared/src/__tests__/profilingCache-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1209,6 +1209,106 @@ describe('ProfilingCache', () => {
}
});

// @reactVersion >= 19.0
it('should detect context changes or lack of changes with conditional use()', () => {
const ContextA = React.createContext(0);
const ContextB = React.createContext(1);
let setState = null;

const Component = () => {
// These hooks may change and initiate re-renders.
let state;
[state, setState] = React.useState('abc');

let result = state;

if (state.includes('a')) {
result += React.use(ContextA);
}

result += React.use(ContextB);

return result;
};

utils.act(() =>
render(
<ContextA.Provider value={1}>
<ContextB.Provider value={1}>
<Component />
</ContextB.Provider>
</ContextA.Provider>,
),
);

utils.act(() => store.profilerStore.startProfiling());

// First render changes Context.
utils.act(() =>
render(
<ContextA.Provider value={0}>
<ContextB.Provider value={1}>
<Component />
</ContextB.Provider>
</ContextA.Provider>,
),
);

// Second render has no changed Context, only changed state.
utils.act(() => setState('def'));

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

const rootID = store.roots[0];

const changeDescriptions = store.profilerStore
.getDataForRoot(rootID)
.commitData.map(commitData => commitData.changeDescriptions);
expect(changeDescriptions).toHaveLength(2);

// 1st render: Change to Context
expect(changeDescriptions[0]).toMatchInlineSnapshot(`
Map {
4 => {
"context": true,
"didHooksChange": false,
"hooks": [],
"isFirstMount": false,
"props": [],
"state": null,
},
}
`);

// 2nd render: Change to State
expect(changeDescriptions[1]).toMatchInlineSnapshot(`
Map {
4 => {
"context": false,
"didHooksChange": true,
"hooks": [
0,
],
"isFirstMount": false,
"props": [],
"state": null,
},
}
`);

expect(changeDescriptions).toHaveLength(2);

// Export and re-import profile data and make sure it is retained.
utils.exportImportHelper(bridge, store);

for (let commitIndex = 0; commitIndex < 2; commitIndex++) {
const commitData = store.profilerStore.getCommitData(rootID, commitIndex);
expect(commitData.changeDescriptions).toEqual(
changeDescriptions[commitIndex],
);
}
});

// @reactVersion >= 18.0
it('should calculate durations based on actual children (not filtered children)', () => {
store.componentFilters = [utils.createDisplayNameFilter('^Parent$')];
Expand Down
8 changes: 8 additions & 0 deletions packages/react-devtools-shared/src/backend/fiber/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -1773,6 +1773,14 @@ export function attach(
// For older versions, there's no good way to read the current context value after render has completed.
// This is because React maintains a stack of context values during render,
// but by the time DevTools is called, render has finished and the stack is empty.
if (prevContext.context !== nextContext.context) {
// If the order of context has changed, then the later context values might have
// changed too but the main reason it rerendered was earlier. Either an earlier
// context changed value but then we would have exited already. If we end up here
// it's because a state or props change caused the order of contexts used to change.
// So the main cause is not the contexts themselves.
return false;
}
if (!is(prevContext.memoizedValue, nextContext.memoizedValue)) {
return true;
}
Expand Down

0 comments on commit d76a565

Please sign in to comment.