refactor[devtools/extension]: more stable element updates polling to avoid timed out errors #27357

hoxyq · 2023-09-11T17:53:03Z

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.

gsathya · 2023-09-12T10:24:40Z

packages/react-devtools-shared/src/inspectedElementCache.js

+}): {abort: () => void, pause: () => void, resume: () => void} {
+  let aborted = false;
+  let paused = false;
+  let running = false;


this seems a bit brittle to have 3 separate state variables, as they could go out of sync in a future refactor. would it make sense to combine them into a single status variable that is either aborted, paused or running?

Great idea, will change this

gsathya · 2023-09-12T10:29:02Z

packages/react-devtools-shared/src/inspectedElementCache.js

+
+    return Promise.allSettled([
+      checkForUpdate({bridge, element, refresh, store}),
+      createPromiseWhichResolvesInOneSecond(),


feel free to ignore, don't feel too strongly: should this per second throttling be part of this or the caller? it's not entirely clear this function does this

The idea here is to throttle checkForUpdate calls: no more than once in 1 second.

Still, if Promise returned by checkForUpdate takes more than 1 second to resolve, Promise.allSettled will only resolve once all Promises are settled, so its basically a safety-check that we would not spam with errors if checkForUpdate instantly returns rejected Promise

Also, if checkForUpdate's Promise will resolve in >1 second, we would not add this 1 second to run next poll request

gsathya · 2023-09-12T10:30:55Z

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


      return () => {
-        clearTimeout(timeoutID);
+        bridge.removeListener('resumeElementPolling', resume);
+        bridge.removeListener('pauseElementPolling', pause);


…avoid timed out errors

* refactor[devtools/extension]: refactored messaging logic across different parts of the extension ([hoxyq](https://github.com/hoxyq) in [#27417](#27417)) * fix[devtools/extension]: added a workaround for proxy content script injection in firefox ([hoxyq](https://github.com/hoxyq) in [#27375](#27375)) * fix[devtools/useTransition]: don't check for dispatch property when determining if hook is stateful ([hoxyq](https://github.com/hoxyq) in [#27365](#27365)) * feat[devtools/extension]: show disclaimer when page doesnt run react and refactor react polling logic ([hoxyq](https://github.com/hoxyq) in [#27373](#27373)) * feat:-Added a delete all filters action and added title to the add filter a… ([Biki-das](https://github.com/Biki-das) in [#27332](#27332)) * fix[devtools/extension]: unregister dynamically injected content scripts instead of filtering ([hoxyq](https://github.com/hoxyq) in [#27369](#27369)) * refactor[devtools/extension]: more stable element updates polling to avoid timed out errors ([hoxyq](https://github.com/hoxyq) in [#27357](#27357)) * feat[devtools/extension]: add dark theme for popup ([rakleed](https://github.com/rakleed) in [#27330](#27330))

…when user switches tabs (#27488) There are not so many changes, most of them are changing imports, because I've moved types for UI in a single file. In #27357 I've added support for pausing polling events: when user inspects an element, we start polling React DevTools backend for updates in props / state. If user switches tabs, extension's service worker can be killed by browser and this polling will start spamming errors. What I've missed is that we also have a separate call for this API, but which is executed only once when user selects an element. We don't handle promise rejection here and this can lead to some errors when user selects an element and switches tabs right after it. The only change here is that this API now has `shouldListenToPauseEvents` param, which is `true` for polling, so we will pause polling once user switches tabs. It is `false` by default, so we won't pause initial call by accident. https://github.com/hoxyq/react/blob/af8beeebf63b5824497fcd0bb35b7c0ac8fe60a0/packages/react-devtools-shared/src/backendAPI.js#L96

…when user switches tabs (facebook#27488) There are not so many changes, most of them are changing imports, because I've moved types for UI in a single file. In facebook#27357 I've added support for pausing polling events: when user inspects an element, we start polling React DevTools backend for updates in props / state. If user switches tabs, extension's service worker can be killed by browser and this polling will start spamming errors. What I've missed is that we also have a separate call for this API, but which is executed only once when user selects an element. We don't handle promise rejection here and this can lead to some errors when user selects an element and switches tabs right after it. The only change here is that this API now has `shouldListenToPauseEvents` param, which is `true` for polling, so we will pause polling once user switches tabs. It is `false` by default, so we won't pause initial call by accident. https://github.com/hoxyq/react/blob/af8beeebf63b5824497fcd0bb35b7c0ac8fe60a0/packages/react-devtools-shared/src/backendAPI.js#L96

…avoid timed out errors (facebook#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.

* refactor[devtools/extension]: refactored messaging logic across different parts of the extension ([hoxyq](https://github.com/hoxyq) in [facebook#27417](facebook#27417)) * fix[devtools/extension]: added a workaround for proxy content script injection in firefox ([hoxyq](https://github.com/hoxyq) in [facebook#27375](facebook#27375)) * fix[devtools/useTransition]: don't check for dispatch property when determining if hook is stateful ([hoxyq](https://github.com/hoxyq) in [facebook#27365](facebook#27365)) * feat[devtools/extension]: show disclaimer when page doesnt run react and refactor react polling logic ([hoxyq](https://github.com/hoxyq) in [facebook#27373](facebook#27373)) * feat:-Added a delete all filters action and added title to the add filter a… ([Biki-das](https://github.com/Biki-das) in [facebook#27332](facebook#27332)) * fix[devtools/extension]: unregister dynamically injected content scripts instead of filtering ([hoxyq](https://github.com/hoxyq) in [facebook#27369](facebook#27369)) * refactor[devtools/extension]: more stable element updates polling to avoid timed out errors ([hoxyq](https://github.com/hoxyq) in [facebook#27357](facebook#27357)) * feat[devtools/extension]: add dark theme for popup ([rakleed](https://github.com/rakleed) in [facebook#27330](facebook#27330))

…when user switches tabs (facebook#27488) There are not so many changes, most of them are changing imports, because I've moved types for UI in a single file. In facebook#27357 I've added support for pausing polling events: when user inspects an element, we start polling React DevTools backend for updates in props / state. If user switches tabs, extension's service worker can be killed by browser and this polling will start spamming errors. What I've missed is that we also have a separate call for this API, but which is executed only once when user selects an element. We don't handle promise rejection here and this can lead to some errors when user selects an element and switches tabs right after it. The only change here is that this API now has `shouldListenToPauseEvents` param, which is `true` for polling, so we will pause polling once user switches tabs. It is `false` by default, so we won't pause initial call by accident. https://github.com/hoxyq/react/blob/af8beeebf63b5824497fcd0bb35b7c0ac8fe60a0/packages/react-devtools-shared/src/backendAPI.js#L96

…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. DiffTrain build for commit 2eed132.

hoxyq requested review from gsathya and motiz88 September 11, 2023 17:53

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Sep 11, 2023

hoxyq force-pushed the devtools/refactor-element-updates-polling branch 2 times, most recently from 1f4d9da to 6e393cc Compare September 12, 2023 10:15

gsathya reviewed Sep 12, 2023

View reviewed changes

gsathya approved these changes Sep 12, 2023

View reviewed changes

refactor[devtools/extension]: more stable element updates polling to …

272b22b

…avoid timed out errors

hoxyq force-pushed the devtools/refactor-element-updates-polling branch from 6e393cc to 272b22b Compare September 12, 2023 13:56

hoxyq merged commit 2eed132 into facebook:main Sep 12, 2023

hoxyq deleted the devtools/refactor-element-updates-polling branch September 12, 2023 14:05

acdlite mentioned this pull request Sep 14, 2023

Update React from dd480ef92 to d6dcad6a8. vercel/next.js#55399

Merged

hoxyq mentioned this pull request Sep 25, 2023

React DevTools 4.28.3 -> 4.28.4 #27419

Merged

hoxyq mentioned this pull request Oct 9, 2023

fix[devtools/inspectElement]: dont pause initial inspectElement call when user switches tabs #27488

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor[devtools/extension]: more stable element updates polling to avoid timed out errors #27357

refactor[devtools/extension]: more stable element updates polling to avoid timed out errors #27357

hoxyq commented Sep 11, 2023

gsathya Sep 12, 2023

hoxyq Sep 12, 2023

gsathya Sep 12, 2023

hoxyq Sep 12, 2023

gsathya Sep 12, 2023

refactor[devtools/extension]: more stable element updates polling to avoid timed out errors #27357

refactor[devtools/extension]: more stable element updates polling to avoid timed out errors #27357

Conversation

hoxyq commented Sep 11, 2023

gsathya Sep 12, 2023

Choose a reason for hiding this comment

hoxyq Sep 12, 2023

Choose a reason for hiding this comment

gsathya Sep 12, 2023

Choose a reason for hiding this comment

hoxyq Sep 12, 2023

Choose a reason for hiding this comment

gsathya Sep 12, 2023

Choose a reason for hiding this comment