Skip to content

Commit

Permalink
[DevTools] Remove lodash.throttle (#30657)
Browse files Browse the repository at this point in the history
Same principle as #30555. We shouldn't be throttling the UI to make it
feel less snappy. Instead, we should use back-pressure to handle it.
Normally the browser handles it automatically with frame aligned events.
E.g. if the thread can't keep up with sync updates it doesn't send each
event but the next one. E.g. pointermove or resize.

However, it is possible that we end up queuing too many events if the
frontend can't keep up but the solution to this is the same as mentioned
in #30555. I.e. to track the last message and only send after we get a
response.

I still keep the throttle to persist the selection since that affects
the disk usage and doesn't have direct UX effects.

The main motivation for this change though is that lodash throttle
doesn't rely on timers but Date.now which makes it incompatible with
most jest helpers which means I can't write tests against these
functions properly.
  • Loading branch information
sebmarkbage authored Aug 12, 2024
1 parent 68dbd84 commit b4c3801
Show file tree
Hide file tree
Showing 4 changed files with 36 additions and 41 deletions.
2 changes: 0 additions & 2 deletions packages/react-devtools-shared/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@
"jsc-safe-url": "^0.2.4",
"json5": "^2.1.3",
"local-storage-fallback": "^4.1.1",
"lodash.throttle": "^4.1.1",
"memoize-one": "^3.1.1",
"react-virtualized-auto-sizer": "^1.0.23",
"react-window": "^1.8.10"
}
Expand Down
48 changes: 28 additions & 20 deletions packages/react-devtools-shared/src/backend/agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
*/

import EventEmitter from '../events';
import throttle from 'lodash.throttle';
import {
SESSION_STORAGE_LAST_SELECTION_KEY,
SESSION_STORAGE_RELOAD_AND_PROFILE_KEY,
Expand Down Expand Up @@ -483,7 +482,13 @@ export default class Agent extends EventEmitter<{
this._persistedSelection = null;
this._persistedSelectionMatch = null;
renderer.setTrackedPath(null);
this._throttledPersistSelection(rendererID, id);
// Throttle persisting the selection.
this._lastSelectedElementID = id;
this._lastSelectedRendererID = rendererID;
if (!this._persistSelectionTimerScheduled) {
this._persistSelectionTimerScheduled = true;
setTimeout(this._persistSelection, 1000);
}
}

// TODO: If there was a way to change the selected DOM element
Expand Down Expand Up @@ -893,22 +898,25 @@ export default class Agent extends EventEmitter<{
this._bridge.send('unsupportedRendererVersion', rendererID);
}

_throttledPersistSelection: any = throttle(
(rendererID: number, id: number) => {
// This is throttled, so both renderer and selected ID
// might not be available by the time we read them.
// This is why we need the defensive checks here.
const renderer = this._rendererInterfaces[rendererID];
const path = renderer != null ? renderer.getPathForElement(id) : null;
if (path !== null) {
sessionStorageSetItem(
SESSION_STORAGE_LAST_SELECTION_KEY,
JSON.stringify(({rendererID, path}: PersistedSelection)),
);
} else {
sessionStorageRemoveItem(SESSION_STORAGE_LAST_SELECTION_KEY);
}
},
1000,
);
_persistSelectionTimerScheduled: boolean = false;
_lastSelectedRendererID: number = -1;
_lastSelectedElementID: number = -1;
_persistSelection: any = () => {
this._persistSelectionTimerScheduled = false;
const rendererID = this._lastSelectedRendererID;
const id = this._lastSelectedElementID;
// This is throttled, so both renderer and selected ID
// might not be available by the time we read them.
// This is why we need the defensive checks here.
const renderer = this._rendererInterfaces[rendererID];
const path = renderer != null ? renderer.getPathForElement(id) : null;
if (path !== null) {
sessionStorageSetItem(
SESSION_STORAGE_LAST_SELECTION_KEY,
JSON.stringify(({rendererID, path}: PersistedSelection)),
);
} else {
sessionStorageRemoveItem(SESSION_STORAGE_LAST_SELECTION_KEY);
}
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@
* @flow
*/

import memoize from 'memoize-one';
import throttle from 'lodash.throttle';
import Agent from 'react-devtools-shared/src/backend/agent';
import {hideOverlay, showOverlay} from './Highlighter';

Expand Down Expand Up @@ -189,18 +187,12 @@ export default function setupHighlighter(
event.stopPropagation();
}

const selectElementForNode = throttle(
memoize((node: HTMLElement) => {
const id = agent.getIDForHostInstance(node);
if (id !== null) {
bridge.send('selectElement', id);
}
}),
200,
// Don't change the selection in the very first 200ms
// because those are usually unintentional as you lift the cursor.
{leading: false},
);
const selectElementForNode = (node: HTMLElement) => {
const id = agent.getIDForHostInstance(node);
if (id !== null) {
bridge.send('selectElement', id);
}
};

function getEventTarget(event: MouseEvent): HTMLElement {
if (event.composed) {
Expand Down
7 changes: 2 additions & 5 deletions packages/react-devtools-shared/src/devtools/views/hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
* @flow
*/

import throttle from 'lodash.throttle';
import {
useCallback,
useEffect,
Expand Down Expand Up @@ -125,10 +124,8 @@ export function useIsOverflowing(

const container = ((containerRef.current: any): HTMLDivElement);

const handleResize = throttle(
() => setIsOverflowing(container.clientWidth <= totalChildWidth),
100,
);
const handleResize = () =>
setIsOverflowing(container.clientWidth <= totalChildWidth);

handleResize();

Expand Down

0 comments on commit b4c3801

Please sign in to comment.