DevTools flushes updated passive warning/error info after delay (#20931)
* DevTools flushes updated passive warning/error info after delay Previously this information was not flushed until the next commit, but this provides a worse user experience if the next commit is really delayed. Instead, the backend now flushes only the warning/error counts after a delay. As a safety, if there are already any pending operations in the queue, we bail. Co-authored-by: eps1lon <silbermann.sebastian@gmail.com>
Brian Vaughn committed
Mar 8, 2021 at 11:33 UTC
e7d2a558ad8e664df36a70b0a86a85f925d1418f
3 files changed
+182
-49
packages/react-devtools-shared/src/__tests__/store-test.js
+98
-28
@@ -1075,40 +1075,110 @@ describe('Store', () => {
1075
`);
1076
});
1077
1078
- // This is not great, but it seems safer than potentially flushing between commits.
1079
- // Our logic for determining how to handle e.g. suspended trees or error boundaries
1080
- // is built on the assumption that we're evaluating the results of a commit, not an in-progress render.
1081
- it('during passive get counted (but not until the next commit)', () => {
1082
- function Example() {
1083
- React.useEffect(() => {
1084
- console.error('test-only: passive error');
1085
- console.warn('test-only: passive warning');
1086
- });
1087
- return null;
1078
+ describe('during passive effects', () => {
1079
+ function flushPendingBridgeOperations() {
1080
+ jest.runOnlyPendingTimers();
1081
}
1089
- const container = document.createElement('div');
1090
-
1091
- withErrorsOrWarningsIgnored(['test-only:'], () => {
1092
- act(() => ReactDOM.render(<Example />, container));
1093
- });
1082
1095
- expect(store).toMatchInlineSnapshot(`
1096
- [root]
1097
- <Example>
1098
- `);
1083
+ // Gross abstraction around pending passive warning/error delay.
1084
+ function flushPendingPassiveErrorAndWarningCounts() {
1085
+ jest.advanceTimersByTime(1000);
1086
+ }
1087
1100
- withErrorsOrWarningsIgnored(['test-only:'], () => {
1101
- act(() => ReactDOM.render(<Example rerender={1} />, container));
1088
+ it('are counted (after a delay)', () => {
1089
+ function Example() {
1090
+ React.useEffect(() => {
1091
+ console.error('test-only: passive error');
1092
+ console.warn('test-only: passive warning');
1093
+ });
1094
+ return null;
1095
+ }
1096
+ const container = document.createElement('div');
1097
+
1098
+ withErrorsOrWarningsIgnored(['test-only:'], () => {
1099
+ act(() => {
1100
+ ReactDOM.render(<Example />, container);
1101
+ }, false);
1102
+ });
1103
+ flushPendingBridgeOperations();
1104
+ expect(store).toMatchInlineSnapshot(`
1105
+ [root]
1106
+ <Example>
1107
+ `);
1108
+
1109
+ // After a delay, passive effects should be committed as well
1110
+ act(flushPendingPassiveErrorAndWarningCounts, false);
1111
+ expect(store).toMatchInlineSnapshot(`
1112
+ ✕ 1, ⚠ 1
1113
+ [root]
1114
+ <Example> ✕⚠
1115
+ `);
1116
+
1117
+ act(() => ReactDOM.unmountComponentAtNode(container));
1118
+ expect(store).toMatchInlineSnapshot(``);
1119
});
1120
1104
- expect(store).toMatchInlineSnapshot(`
1105
- ✕ 1, ⚠ 1
1106
- [root]
1107
- <Example> ✕⚠
1108
- `);
1121
+ it('are flushed early when there is a new commit', () => {
1122
+ function Example() {
1123
+ React.useEffect(() => {
1124
+ console.error('test-only: passive error');
1125
+ console.warn('test-only: passive warning');
1126
+ });
1127
+ return null;
1128
+ }
1129
+
1130
+ function Noop() {
1131
+ return null;
1132
+ }
1133
+
1134
+ const container = document.createElement('div');
1135
+
1136
+ withErrorsOrWarningsIgnored(['test-only:'], () => {
1137
+ act(() => {
1138
+ ReactDOM.render(
1139
+ <>
1140
+ <Example />
1141
+ </>,
1142
+ container,
1143
+ );
1144
+ }, false);
1145
+ flushPendingBridgeOperations();
1146
+ expect(store).toMatchInlineSnapshot(`
1147
+ [root]
1148
+ <Example>
1149
+ `);
1150
+
1151
+ // Before warnings and errors have flushed, flush another commit.
1152
+ act(() => {
1153
+ ReactDOM.render(
1154
+ <>
1155
+ <Example />
1156
+ <Noop />
1157
+ </>,
1158
+ container,
1159
+ );
1160
+ }, false);
1161
+ flushPendingBridgeOperations();
1162
+ expect(store).toMatchInlineSnapshot(`
1163
+ ✕ 1, ⚠ 1
1164
+ [root]
1165
+ <Example> ✕⚠
1166
+ <Noop>
1167
+ `);
1168
+ });
1169
1110
- act(() => ReactDOM.unmountComponentAtNode(container));
1111
- expect(store).toMatchInlineSnapshot(``);
1170
+ // After a delay, passive effects should be committed as well
1171
+ act(flushPendingPassiveErrorAndWarningCounts, false);
1172
+ expect(store).toMatchInlineSnapshot(`
1173
+ ✕ 2, ⚠ 2
1174
+ [root]
1175
+ <Example> ✕⚠
1176
+ <Noop>
1177
+ `);
1178
+
1179
+ act(() => ReactDOM.unmountComponentAtNode(container));
1180
+ expect(store).toMatchInlineSnapshot(``);
1181
+ });
1182
});
1183
1184
it('from react get counted', () => {
packages/react-devtools-shared/src/__tests__/utils.js
+12
-7
@@ -14,7 +14,10 @@ import type Store from 'react-devtools-shared/src/devtools/store';
14
import type {ProfilingDataFrontend} from 'react-devtools-shared/src/devtools/views/Profiler/types';
15
import type {ElementType} from 'react-devtools-shared/src/types';
16
17
-export function act(callback: Function): void {
17
+export function act(
18
+ callback: Function,
19
+ recursivelyFlush: boolean = true,
20
+): void {
21
const {act: actTestRenderer} = require('react-test-renderer');
22
const {act: actDOM} = require('react-dom/test-utils');
23
@@ -24,13 +27,15 @@ export function act(callback: Function): void {
27
});
28
});
29
27
- // Flush Bridge operations
28
- while (jest.getTimerCount() > 0) {
29
- actDOM(() => {
30
- actTestRenderer(() => {
31
- jest.runAllTimers();
30
+ if (recursivelyFlush) {
31
+ // Flush Bridge operations
32
+ while (jest.getTimerCount() > 0) {
33
+ actDOM(() => {
34
+ actTestRenderer(() => {
35
+ jest.runAllTimers();
36
+ });
37
});
33
- });
38
+ }
39
}
40
}
41
packages/react-devtools-shared/src/backend/renderer.js
+72
-14
@@ -626,6 +626,14 @@ export function attach(
626
627
// If this Fiber is currently being inspected, mark it as needing an udpate as well.
628
updateMostRecentlyInspectedElementIfNecessary(fiberID);
629
+
630
+ // Passive effects may trigger errors or warnings too;
631
+ // In this case, we should wait until the rest of the passive effects have run,
632
+ // but we shouldn't wait until the next commit because that might be a long time.
633
+ // This would also cause "tearing" between an inspected Component and the tree view.
634
+ // Then again we don't want to flush too soon because this could be an error during async rendering.
635
+ // Use a debounce technique to ensure that we'll eventually flush.
636
+ flushPendingErrorsAndWarningsAfterDelay();
637
}
638
639
// Patching the console enables DevTools to do a few useful things:
@@ -1198,10 +1206,12 @@ export function attach(
1206
}
1207
}
1208
1201
- const pendingOperations: Array<number> = [];
1209
+ type OperationsArray = Array<number>;
1210
+
1211
+ const pendingOperations: OperationsArray = [];
1212
const pendingRealUnmountedIDs: Array<number> = [];
1213
const pendingSimulatedUnmountedIDs: Array<number> = [];
1204
- let pendingOperationsQueue: Array<Array<number>> | null = [];
1214
+ let pendingOperationsQueue: Array<OperationsArray> | null = [];
1215
const pendingStringTable: Map<string, number> = new Map();
1216
let pendingStringTableLength: number = 0;
1217
let pendingUnmountedRootID: number | null = null;
@@ -1218,6 +1228,61 @@ export function attach(
1228
pendingOperations.push(op);
1229
}
1230
1231
+ function flushOrQueueOperations(operations: OperationsArray): void {
1232
+ if (pendingOperationsQueue !== null) {
1233
+ pendingOperationsQueue.push(operations);
1234
+ } else {
1235
+ hook.emit('operations', operations);
1236
+ }
1237
+ }
1238
+
1239
+ let flushPendingErrorsAndWarningsAfterDelayTimeoutID = null;
1240
+
1241
+ function clearPendingErrorsAndWarningsAfterDelay() {
1242
+ if (flushPendingErrorsAndWarningsAfterDelayTimeoutID !== null) {
1243
+ clearTimeout(flushPendingErrorsAndWarningsAfterDelayTimeoutID);
1244
+ flushPendingErrorsAndWarningsAfterDelayTimeoutID = null;
1245
+ }
1246
+ }
1247
+
1248
+ function flushPendingErrorsAndWarningsAfterDelay() {
1249
+ clearPendingErrorsAndWarningsAfterDelay();
1250
+
1251
+ flushPendingErrorsAndWarningsAfterDelayTimeoutID = setTimeout(() => {
1252
+ flushPendingErrorsAndWarningsAfterDelayTimeoutID = null;
1253
+
1254
+ if (pendingOperations.length > 0) {
1255
+ // On the off chance that something else has pushed pending operations,
1256
+ // we should bail on warnings; it's probably not safe to push midway.
1257
+ return;
1258
+ }
1259
+
1260
+ recordPendingErrorsAndWarnings();
1261
+
1262
+ if (pendingOperations.length === 0) {
1263
+ // No warnings or errors to flush; we can bail out early here too.
1264
+ return;
1265
+ }
1266
+
1267
+ // We can create a smaller operations array than flushPendingEvents()
1268
+ // because we only need to flush warning and error counts.
1269
+ // Only a few pieces of fixed information are required up front.
1270
+ const operations: OperationsArray = new Array(
1271
+ 3 + pendingOperations.length,
1272
+ );
1273
+ operations[0] = rendererID;
1274
+ operations[1] = currentRootID;
1275
+ operations[2] = 0; // String table size
1276
+ for (let j = 0; j < pendingOperations.length; j++) {
1277
+ operations[3 + j] = pendingOperations[j];
1278
+ }
1279
+
1280
+ flushOrQueueOperations(operations);
1281
+
1282
+ pendingOperations.length = 0;
1283
+ }, 1000);
1284
+ }
1285
+
1286
function reevaluateErrorsAndWarnings() {
1287
fibersWithChangedErrorOrWarningCounts.clear();
1288
fiberToErrorsMap.forEach((countMap, fiberID) => {
@@ -1230,6 +1295,8 @@ export function attach(
1295
}
1296
1297
function recordPendingErrorsAndWarnings() {
1298
+ clearPendingErrorsAndWarningsAfterDelay();
1299
+
1300
fibersWithChangedErrorOrWarningCounts.forEach(fiberID => {
1301
const fiber = idToFiberMap.get(fiberID);
1302
if (fiber != null) {
@@ -1319,7 +1386,7 @@ export function attach(
1386
// Which in turn enables fiber props, states, and hooks to be inspected.
1387
let i = 0;
1388
operations[i++] = rendererID;
1322
- operations[i++] = currentRootID; // Use this ID in case the root was unmounted!
1389
+ operations[i++] = currentRootID;
1390
1391
// Now fill in the string table.
1392
// [stringTableLength, str1Length, ...str1, str2Length, ...str2, ...]
@@ -1366,18 +1433,9 @@ export function attach(
1433
i += pendingOperations.length;
1434
1435
// Let the frontend know about tree operations.
1369
- // The first value in this array will identify which root it corresponds to,
1370
- // so we do no longer need to dispatch a separate root-committed event.
1371
- if (pendingOperationsQueue !== null) {
1372
- // Until the frontend has been connected, store the tree operations.
1373
- // This will let us avoid walking the tree later when the frontend connects,
1374
- // and it enables the Profiler's reload-and-profile functionality to work as well.
1375
- pendingOperationsQueue.push(operations);
1376
- } else {
1377
- // If we've already connected to the frontend, just pass the operations through.
1378
- hook.emit('operations', operations);
1379
- }
1436
+ flushOrQueueOperations(operations);
1437
1438
+ // Reset all of the pending state now that we've told the frontend about it.
1439
pendingOperations.length = 0;
1440
pendingRealUnmountedIDs.length = 0;
1441
pendingSimulatedUnmountedIDs.length = 0;