Skip to content

Commit 2eed132

Browse files
hoxyqhoxyq
authored
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.
1 parent bb1d8d1 commit 2eed132

File tree

7 files changed

+158
-35
lines changed

7 files changed

+158
-35
lines changed

‎packages/react-devtools-extensions/src/background/index.js‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ chrome.runtime.onConnect.addListener(port => {
8383
}
8484

8585
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.
8787
const tabId = +port.name;
8888

8989
registerTab(tabId);
@@ -231,3 +231,20 @@ chrome.runtime.onMessage.addListener((message, sender) => {
231231
}
232232
}
233233
});
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‎

Lines changed: 7 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -461,10 +461,6 @@ describe('InspectedElement', () => {
461461
// This test causes an intermediate error to be logged but we can ignore it.
462462
jest.spyOn(console, 'error').mockImplementation(() => {});
463463

464-
// Wait for our check-for-updates poll to get the new data.
465-
jest.runOnlyPendingTimers();
466-
await Promise.resolve();
467-
468464
// Clear the frontend cache to simulate DevTools being closed and re-opened.
469465
// The backend still thinks the most recently-inspected element is still cached,
470466
// so the frontend needs to tell it to resend a full value.
@@ -1072,7 +1068,6 @@ describe('InspectedElement', () => {
10721068
await TestUtilsAct(async () => {
10731069
await TestRendererAct(async () => {
10741070
inspectElementPath(path);
1075-
jest.runOnlyPendingTimers();
10761071
});
10771072
});
10781073

@@ -1227,7 +1222,6 @@ describe('InspectedElement', () => {
12271222
await TestUtilsAct(async () => {
12281223
await TestRendererAct(async () => {
12291224
inspectElementPath(path);
1230-
jest.runOnlyPendingTimers();
12311225
});
12321226
});
12331227

@@ -1309,7 +1303,6 @@ describe('InspectedElement', () => {
13091303
await TestUtilsAct(async () => {
13101304
await TestRendererAct(async () => {
13111305
inspectElementPath(path);
1312-
jest.runOnlyPendingTimers();
13131306
});
13141307
});
13151308

@@ -1470,9 +1463,8 @@ describe('InspectedElement', () => {
14701463

14711464
async function loadPath(path) {
14721465
await TestUtilsAct(async () => {
1473-
await TestRendererAct(async () => {
1466+
await TestRendererAct(() => {
14741467
inspectElementPath(path);
1475-
jest.runOnlyPendingTimers();
14761468
});
14771469
});
14781470

@@ -1597,9 +1589,8 @@ describe('InspectedElement', () => {
15971589

15981590
async function loadPath(path) {
15991591
await TestUtilsAct(async () => {
1600-
await TestRendererAct(async () => {
1592+
await TestRendererAct(() => {
16011593
inspectElementPath(path);
1602-
jest.runOnlyPendingTimers();
16031594
});
16041595
});
16051596

@@ -1640,9 +1631,11 @@ describe('InspectedElement', () => {
16401631
expect(inspectedElement.props).toMatchInlineSnapshot(`
16411632
{
16421633
"nestedObject": {
1643-
"a": Dehydrated {
1644-
"preview_short": {…},
1645-
"preview_long": {b: {…}, value: 2},
1634+
"a": {
1635+
"b": {
1636+
"value": 2,
1637+
},
1638+
"value": 2,
16461639
},
16471640
"value": 2,
16481641
},

‎packages/react-devtools-shared/src/backendAPI.js‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {hydrate, fillInPath} from 'react-devtools-shared/src/hydration';
1111
import {separateDisplayNameAndHOCs} from 'react-devtools-shared/src/utils';
1212
import Store from 'react-devtools-shared/src/devtools/store';
1313
import TimeoutError from 'react-devtools-shared/src/errors/TimeoutError';
14+
import ElementPollingCancellationError from 'react-devtools-shared/src/errors/ElementPollingCancellationError';
1415

1516
import type {
1617
InspectedElement as InspectedElementBackend,
@@ -138,7 +139,7 @@ export function storeAsGlobal({
138139
});
139140
}
140141

141-
const TIMEOUT_DELAY = 5000;
142+
const TIMEOUT_DELAY = 10_000;
142143

143144
let requestCounter = 0;
144145

@@ -151,10 +152,17 @@ function getPromiseForRequestID<T>(
151152
return new Promise((resolve, reject) => {
152153
const cleanup = () => {
153154
bridge.removeListener(eventType, onInspectedElement);
155+
bridge.removeListener('shutdown', onDisconnect);
156+
bridge.removeListener('pauseElementPolling', onDisconnect);
154157

155158
clearTimeout(timeoutID);
156159
};
157160

161+
const onDisconnect = () => {
162+
cleanup();
163+
reject(new ElementPollingCancellationError());
164+
};
165+
158166
const onInspectedElement = (data: any) => {
159167
if (data.responseID === requestID) {
160168
cleanup();
@@ -168,6 +176,8 @@ function getPromiseForRequestID<T>(
168176
};
169177

170178
bridge.addListener(eventType, onInspectedElement);
179+
bridge.addListener('shutdown', onDisconnect);
180+
bridge.addListener('pauseElementPolling', onDisconnect);
171181

172182
const timeoutID = setTimeout(onTimeout, TIMEOUT_DELAY);
173183
});

‎packages/react-devtools-shared/src/bridge.js‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -263,6 +263,9 @@ type FrontendEvents = {
263263
overrideHookState: [OverrideHookState],
264264
overrideProps: [OverrideValue],
265265
overrideState: [OverrideValue],
266+
267+
resumeElementPolling: [],
268+
pauseElementPolling: [],
266269
};
267270

268271
class Bridge<

‎packages/react-devtools-shared/src/devtools/views/Components/InspectedElementContext.js‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,8 @@ import {
2424
import {TreeStateContext} from './TreeContext';
2525
import {BridgeContext, StoreContext} from '../context';
2626
import {
27-
checkForUpdate,
2827
inspectElement,
28+
startElementUpdatesPolling,
2929
} from 'react-devtools-shared/src/inspectedElementCache';
3030
import {
3131
clearHookNamesCache,
@@ -59,8 +59,6 @@ type Context = {
5959
export const InspectedElementContext: ReactContext<Context> =
6060
createContext<Context>(((null: any): Context));
6161

62-
const POLL_INTERVAL = 1000;
63-
6462
export type Props = {
6563
children: ReactNodeList,
6664
};
@@ -228,14 +226,21 @@ export function InspectedElementContextController({
228226
// Periodically poll the selected element for updates.
229227
useEffect(() => {
230228
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);
236238

237239
return () => {
238-
clearTimeout(timeoutID);
240+
bridge.removeListener('resumeElementPolling', resume);
241+
bridge.removeListener('pauseElementPolling', pause);
242+
243+
abort();
239244
};
240245
}
241246
}, [

‎packages/react-devtools-shared/src/errors/ElementPollingCancellationError.js‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -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‎

Lines changed: 84 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,9 @@ import {
1111
unstable_getCacheForType as getCacheForType,
1212
startTransition,
1313
} 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';
1617

1718
import type {FrontendBridge} from 'react-devtools-shared/src/bridge';
1819
import type {Wakeable} from 'shared/ReactTypes';
@@ -177,15 +178,15 @@ export function checkForUpdate({
177178
element: Element,
178179
refresh: RefreshFunction,
179180
store: Store,
180-
}): void {
181+
}): void | Promise<void> {
181182
const {id} = element;
182183
const rendererID = store.getRendererIDForElement(id);
183184

184185
if (rendererID == null) {
185186
return;
186187
}
187188

188-
inspectElementMutableSource({
189+
return inspectElementMutableSource({
189190
bridge,
190191
element,
191192
path: null,
@@ -202,15 +203,88 @@ export function checkForUpdate({
202203
});
203204
}
204205
},
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-
},
211206
);
212207
}
213208

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+
214288
export function clearCacheBecauseOfError(refresh: RefreshFunction): void {
215289
startTransition(() => {
216290
const map = createMap();

0 commit comments

Comments
 (0)