[DevTools] Handle reordered contexts in Profiler (#30887)
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.
Sebastian Markbåge committed
Sep 6, 2024 at 21:45 UTC
d76a5651f46619fde486b4119953a8f9dd240612
2 files changed
+108
packages/react-devtools-shared/src/__tests__/profilingCache-test.js
+100
@@ -1209,6 +1209,106 @@ describe('ProfilingCache', () => {
1209
}
1210
});
1211
1212
+ // @reactVersion >= 19.0
1213
+ it('should detect context changes or lack of changes with conditional use()', () => {
1214
+ const ContextA = React.createContext(0);
1215
+ const ContextB = React.createContext(1);
1216
+ let setState = null;
1217
+
1218
+ const Component = () => {
1219
+ // These hooks may change and initiate re-renders.
1220
+ let state;
1221
+ [state, setState] = React.useState('abc');
1222
+
1223
+ let result = state;
1224
+
1225
+ if (state.includes('a')) {
1226
+ result += React.use(ContextA);
1227
+ }
1228
+
1229
+ result += React.use(ContextB);
1230
+
1231
+ return result;
1232
+ };
1233
+
1234
+ utils.act(() =>
1235
+ render(
1236
+ <ContextA.Provider value={1}>
1237
+ <ContextB.Provider value={1}>
1238
+ <Component />
1239
+ </ContextB.Provider>
1240
+ </ContextA.Provider>,
1241
+ ),
1242
+ );
1243
+
1244
+ utils.act(() => store.profilerStore.startProfiling());
1245
+
1246
+ // First render changes Context.
1247
+ utils.act(() =>
1248
+ render(
1249
+ <ContextA.Provider value={0}>
1250
+ <ContextB.Provider value={1}>
1251
+ <Component />
1252
+ </ContextB.Provider>
1253
+ </ContextA.Provider>,
1254
+ ),
1255
+ );
1256
+
1257
+ // Second render has no changed Context, only changed state.
1258
+ utils.act(() => setState('def'));
1259
+
1260
+ utils.act(() => store.profilerStore.stopProfiling());
1261
+
1262
+ const rootID = store.roots[0];
1263
+
1264
+ const changeDescriptions = store.profilerStore
1265
+ .getDataForRoot(rootID)
1266
+ .commitData.map(commitData => commitData.changeDescriptions);
1267
+ expect(changeDescriptions).toHaveLength(2);
1268
+
1269
+ // 1st render: Change to Context
1270
+ expect(changeDescriptions[0]).toMatchInlineSnapshot(`
1271
+ Map {
1272
+ 4 => {
1273
+ "context": true,
1274
+ "didHooksChange": false,
1275
+ "hooks": [],
1276
+ "isFirstMount": false,
1277
+ "props": [],
1278
+ "state": null,
1279
+ },
1280
+ }
1281
+ `);
1282
+
1283
+ // 2nd render: Change to State
1284
+ expect(changeDescriptions[1]).toMatchInlineSnapshot(`
1285
+ Map {
1286
+ 4 => {
1287
+ "context": false,
1288
+ "didHooksChange": true,
1289
+ "hooks": [
1290
+ 0,
1291
+ ],
1292
+ "isFirstMount": false,
1293
+ "props": [],
1294
+ "state": null,
1295
+ },
1296
+ }
1297
+ `);
1298
+
1299
+ expect(changeDescriptions).toHaveLength(2);
1300
+
1301
+ // Export and re-import profile data and make sure it is retained.
1302
+ utils.exportImportHelper(bridge, store);
1303
+
1304
+ for (let commitIndex = 0; commitIndex < 2; commitIndex++) {
1305
+ const commitData = store.profilerStore.getCommitData(rootID, commitIndex);
1306
+ expect(commitData.changeDescriptions).toEqual(
1307
+ changeDescriptions[commitIndex],
1308
+ );
1309
+ }
1310
+ });
1311
+
1312
// @reactVersion >= 18.0
1313
it('should calculate durations based on actual children (not filtered children)', () => {
1314
store.componentFilters = [utils.createDisplayNameFilter('^Parent$')];
packages/react-devtools-shared/src/backend/fiber/renderer.js
+8
@@ -1773,6 +1773,14 @@ export function attach(
1773
// For older versions, there's no good way to read the current context value after render has completed.
1774
// This is because React maintains a stack of context values during render,
1775
// but by the time DevTools is called, render has finished and the stack is empty.
1776
+ if (prevContext.context !== nextContext.context) {
1777
+ // If the order of context has changed, then the later context values might have
1778
+ // changed too but the main reason it rerendered was earlier. Either an earlier
1779
+ // context changed value but then we would have exited already. If we end up here
1780
+ // it's because a state or props change caused the order of contexts used to change.
1781
+ // So the main cause is not the contexts themselves.
1782
+ return false;
1783
+ }
1784
if (!is(prevContext.memoizedValue, nextContext.memoizedValue)) {
1785
return true;
1786
}