refactor[devtools/extension]: more stable element updates polling to avoid timed out errors (#27357)
Some context: - When user selects an element in tree inspector, we display current state of the component. In order to display really current state, we start polling the backend to get available updates for the element. Previously: - Straight-forward sending an event to get element updates each second. Potential race condition is not handled in any form. - If user navigates from the page, timeout wouldn't be cleared and we would potentially throw "Timed out ..." error. - Bridge disconnection is not handled in any form, if it was shut down, we could spam with "Timed out ..." errors. With these changes: - Requests are now chained, so there can be a single request at a time. - Handling both navigation and shut down events. This should reduce the number of "Timed out ..." errors that we see in our logs for the extension. Other surfaces will also benefit from it, but not to the full extent, as long as they utilize "resumeElementPolling" and "pauseElementPolling" events. Tested this on Chrome, running React DevTools on multiple tabs, explicitly checked the case when service worker is in idle state and we return back to the tab.
Ruslan Lesiutin committed
Sep 12, 2023 at 15:05 UTC
2eed1328478e8c923fcb4e6abf5efbd9e1233402
7 files changed
+158
-35
packages/react-devtools-extensions/src/background/index.js
+18
-1
@@ -83,7 +83,7 @@ chrome.runtime.onConnect.addListener(port => {
83
}
84
85
if (isNumeric(port.name)) {
86
- // Extension port doesn't have tab id specified, because its sender is the extension.
86
+ // DevTools page port doesn't have tab id specified, because its sender is the extension.
87
const tabId = +port.name;
88
89
registerTab(tabId);
@@ -231,3 +231,20 @@ chrome.runtime.onMessage.addListener((message, sender) => {
231
}
232
}
233
});
234
+
235
+chrome.tabs.onActivated.addListener(({tabId: activeTabId}) => {
236
+ for (const registeredTabId in ports) {
237
+ if (
238
+ ports[registeredTabId].proxy != null &&
239
+ ports[registeredTabId].extension != null
240
+ ) {
241
+ const numericRegisteredTabId = +registeredTabId;
242
+ const event =
243
+ activeTabId === numericRegisteredTabId
244
+ ? 'resumeElementPolling'
245
+ : 'pauseElementPolling';
246
+
247
+ ports[registeredTabId].extension.postMessage({event});
248
+ }
249
+ }
250
+});
packages/react-devtools-shared/src/__tests__/inspectedElement-test.js
+7
-14
@@ -461,10 +461,6 @@ describe('InspectedElement', () => {
461
// This test causes an intermediate error to be logged but we can ignore it.
462
jest.spyOn(console, 'error').mockImplementation(() => {});
463
464
- // Wait for our check-for-updates poll to get the new data.
465
- jest.runOnlyPendingTimers();
466
- await Promise.resolve();
467
-
464
// Clear the frontend cache to simulate DevTools being closed and re-opened.
465
// The backend still thinks the most recently-inspected element is still cached,
466
// so the frontend needs to tell it to resend a full value.
@@ -1072,7 +1068,6 @@ describe('InspectedElement', () => {
1068
await TestUtilsAct(async () => {
1069
await TestRendererAct(async () => {
1070
inspectElementPath(path);
1075
- jest.runOnlyPendingTimers();
1071
});
1072
});
1073
@@ -1227,7 +1222,6 @@ describe('InspectedElement', () => {
1222
await TestUtilsAct(async () => {
1223
await TestRendererAct(async () => {
1224
inspectElementPath(path);
1230
- jest.runOnlyPendingTimers();
1225
});
1226
});
1227
@@ -1309,7 +1303,6 @@ describe('InspectedElement', () => {
1303
await TestUtilsAct(async () => {
1304
await TestRendererAct(async () => {
1305
inspectElementPath(path);
1312
- jest.runOnlyPendingTimers();
1306
});
1307
});
1308
@@ -1470,9 +1463,8 @@ describe('InspectedElement', () => {
1463
1464
async function loadPath(path) {
1465
await TestUtilsAct(async () => {
1473
- await TestRendererAct(async () => {
1466
+ await TestRendererAct(() => {
1467
inspectElementPath(path);
1475
- jest.runOnlyPendingTimers();
1468
});
1469
});
1470
@@ -1597,9 +1589,8 @@ describe('InspectedElement', () => {
1589
1590
async function loadPath(path) {
1591
await TestUtilsAct(async () => {
1600
- await TestRendererAct(async () => {
1592
+ await TestRendererAct(() => {
1593
inspectElementPath(path);
1602
- jest.runOnlyPendingTimers();
1594
});
1595
});
1596
@@ -1640,9 +1631,11 @@ describe('InspectedElement', () => {
1631
expect(inspectedElement.props).toMatchInlineSnapshot(`
1632
{
1633
"nestedObject": {
1643
- "a": Dehydrated {
1644
- "preview_short": {…},
1645
- "preview_long": {b: {…}, value: 2},
1634
+ "a": {
1635
+ "b": {
1636
+ "value": 2,
1637
+ },
1638
+ "value": 2,
1639
},
1640
"value": 2,
1641
},
packages/react-devtools-shared/src/backendAPI.js
+11
-1
@@ -11,6 +11,7 @@ import {hydrate, fillInPath} from 'react-devtools-shared/src/hydration';
11
import {separateDisplayNameAndHOCs} from 'react-devtools-shared/src/utils';
12
import Store from 'react-devtools-shared/src/devtools/store';
13
import TimeoutError from 'react-devtools-shared/src/errors/TimeoutError';
14
+import ElementPollingCancellationError from 'react-devtools-shared/src/errors/ElementPollingCancellationError';
15
16
import type {
17
InspectedElement as InspectedElementBackend,
@@ -138,7 +139,7 @@ export function storeAsGlobal({
139
});
140
}
141
141
-const TIMEOUT_DELAY = 5000;
142
+const TIMEOUT_DELAY = 10_000;
143
144
let requestCounter = 0;
145
@@ -151,10 +152,17 @@ function getPromiseForRequestID<T>(
152
return new Promise((resolve, reject) => {
153
const cleanup = () => {
154
bridge.removeListener(eventType, onInspectedElement);
155
+ bridge.removeListener('shutdown', onDisconnect);
156
+ bridge.removeListener('pauseElementPolling', onDisconnect);
157
158
clearTimeout(timeoutID);
159
};
160
161
+ const onDisconnect = () => {
162
+ cleanup();
163
+ reject(new ElementPollingCancellationError());
164
+ };
165
+
166
const onInspectedElement = (data: any) => {
167
if (data.responseID === requestID) {
168
cleanup();
@@ -168,6 +176,8 @@ function getPromiseForRequestID<T>(
176
};
177
178
bridge.addListener(eventType, onInspectedElement);
179
+ bridge.addListener('shutdown', onDisconnect);
180
+ bridge.addListener('pauseElementPolling', onDisconnect);
181
182
const timeoutID = setTimeout(onTimeout, TIMEOUT_DELAY);
183
});
packages/react-devtools-shared/src/bridge.js
+3
@@ -263,6 +263,9 @@ type FrontendEvents = {
263
overrideHookState: [OverrideHookState],
264
overrideProps: [OverrideValue],
265
overrideState: [OverrideValue],
266
+
267
+ resumeElementPolling: [],
268
+ pauseElementPolling: [],
269
};
270
271
class Bridge<
packages/react-devtools-shared/src/devtools/views/Components/InspectedElementContext.js
+14
-9
@@ -24,8 +24,8 @@ import {
24
import {TreeStateContext} from './TreeContext';
25
import {BridgeContext, StoreContext} from '../context';
26
import {
27
- checkForUpdate,
27
inspectElement,
28
+ startElementUpdatesPolling,
29
} from 'react-devtools-shared/src/inspectedElementCache';
30
import {
31
clearHookNamesCache,
@@ -59,8 +59,6 @@ type Context = {
59
export const InspectedElementContext: ReactContext<Context> =
60
createContext<Context>(((null: any): Context));
61
62
-const POLL_INTERVAL = 1000;
63
-
62
export type Props = {
63
children: ReactNodeList,
64
};
@@ -228,14 +226,21 @@ export function InspectedElementContextController({
226
// Periodically poll the selected element for updates.
227
useEffect(() => {
228
if (element !== null && bridgeIsAlive) {
231
- const checkForUpdateWrapper = () => {
232
- checkForUpdate({bridge, element, refresh, store});
233
- timeoutID = setTimeout(checkForUpdateWrapper, POLL_INTERVAL);
234
- };
235
- let timeoutID = setTimeout(checkForUpdateWrapper, POLL_INTERVAL);
229
+ const {abort, pause, resume} = startElementUpdatesPolling({
230
+ bridge,
231
+ element,
232
+ refresh,
233
+ store,
234
+ });
235
+
236
+ bridge.addListener('resumeElementPolling', resume);
237
+ bridge.addListener('pauseElementPolling', pause);
238
239
return () => {
238
- clearTimeout(timeoutID);
240
+ bridge.removeListener('resumeElementPolling', resume);
241
+ bridge.removeListener('pauseElementPolling', pause);
242
+
243
+ abort();
244
};
245
}
246
}, [
packages/react-devtools-shared/src/errors/ElementPollingCancellationError.js
new
+21
@@ -0,0 +1,21 @@
1
+/**
2
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
3
+ *
4
+ * This source code is licensed under the MIT license found in the
5
+ * LICENSE file in the root directory of this source tree.
6
+ *
7
+ * @flow
8
+ */
9
+
10
+export default class ElementPollingCancellationError extends Error {
11
+ constructor() {
12
+ super();
13
+
14
+ // Maintains proper stack trace for where our error was thrown (only available on V8)
15
+ if (Error.captureStackTrace) {
16
+ Error.captureStackTrace(this, ElementPollingCancellationError);
17
+ }
18
+
19
+ this.name = 'ElementPollingCancellationError';
20
+ }
21
+}
packages/react-devtools-shared/src/inspectedElementCache.js
+84
-10
@@ -11,8 +11,9 @@ import {
11
unstable_getCacheForType as getCacheForType,
12
startTransition,
13
} from 'react';
14
-import Store from './devtools/store';
15
-import {inspectElement as inspectElementMutableSource} from './inspectedElementMutableSource';
14
+import Store from 'react-devtools-shared/src/devtools/store';
15
+import {inspectElement as inspectElementMutableSource} from 'react-devtools-shared/src/inspectedElementMutableSource';
16
+import ElementPollingCancellationError from 'react-devtools-shared/src//errors/ElementPollingCancellationError';
17
18
import type {FrontendBridge} from 'react-devtools-shared/src/bridge';
19
import type {Wakeable} from 'shared/ReactTypes';
@@ -177,7 +178,7 @@ export function checkForUpdate({
178
element: Element,
179
refresh: RefreshFunction,
180
store: Store,
180
-}): void {
181
+}): void | Promise<void> {
182
const {id} = element;
183
const rendererID = store.getRendererIDForElement(id);
184
@@ -185,7 +186,7 @@ export function checkForUpdate({
186
return;
187
}
188
188
- inspectElementMutableSource({
189
+ return inspectElementMutableSource({
190
bridge,
191
element,
192
path: null,
@@ -202,15 +203,88 @@ export function checkForUpdate({
203
});
204
}
205
},
205
-
206
- // There isn't much to do about errors in this case,
207
- // but we should at least log them so they aren't silent.
208
- error => {
209
- console.error(error);
210
- },
206
);
207
}
208
209
+function createPromiseWhichResolvesInOneSecond() {
210
+ return new Promise(resolve => setTimeout(resolve, 1000));
211
+}
212
+
213
+type PollingStatus = 'idle' | 'running' | 'paused' | 'aborted';
214
+
215
+export function startElementUpdatesPolling({
216
+ bridge,
217
+ element,
218
+ refresh,
219
+ store,
220
+}: {
221
+ bridge: FrontendBridge,
222
+ element: Element,
223
+ refresh: RefreshFunction,
224
+ store: Store,
225
+}): {abort: () => void, pause: () => void, resume: () => void} {
226
+ let status: PollingStatus = 'idle';
227
+
228
+ function abort() {
229
+ status = 'aborted';
230
+ }
231
+
232
+ function resume() {
233
+ if (status === 'running' || status === 'aborted') {
234
+ return;
235
+ }
236
+
237
+ status = 'idle';
238
+ poll();
239
+ }
240
+
241
+ function pause() {
242
+ if (status === 'paused' || status === 'aborted') {
243
+ return;
244
+ }
245
+
246
+ status = 'paused';
247
+ }
248
+
249
+ function poll(): Promise<void> {
250
+ status = 'running';
251
+
252
+ return Promise.allSettled([
253
+ checkForUpdate({bridge, element, refresh, store}),
254
+ createPromiseWhichResolvesInOneSecond(),
255
+ ])
256
+ .then(([{status: updateStatus, reason}]) => {
257
+ // There isn't much to do about errors in this case,
258
+ // but we should at least log them, so they aren't silent.
259
+ // Log only if polling is still active, we can't handle the case when
260
+ // request was sent, and then bridge was remounted (for example, when user did navigate to a new page),
261
+ // but at least we can mark that polling was aborted
262
+ if (updateStatus === 'rejected' && status !== 'aborted') {
263
+ // This is expected Promise rejection, no need to log it
264
+ if (reason instanceof ElementPollingCancellationError) {
265
+ return;
266
+ }
267
+
268
+ console.error(reason);
269
+ }
270
+ })
271
+ .finally(() => {
272
+ const shouldContinuePolling =
273
+ status !== 'aborted' && status !== 'paused';
274
+
275
+ status = 'idle';
276
+
277
+ if (shouldContinuePolling) {
278
+ return poll();
279
+ }
280
+ });
281
+ }
282
+
283
+ poll();
284
+
285
+ return {abort, resume, pause};
286
+}
287
+
288
export function clearCacheBecauseOfError(refresh: RefreshFunction): void {
289
startTransition(() => {
290
const map = createMap();