DevTools: Replaced WeakMap with LRU for inspected element cache (#22160)
We use an LRU for this rather than a WeakMap because of how the "no-change" optimization works. When the frontend polls the backend for an update on the element that's currently inspected, the backend will send a "no-change" message if the element hasn't updated (rendered) since the last time it was asked. In thid case, the frontend cache should reuse the previous (cached) value. Using a WeakMap keyed on Element generally works well for this, since Elements are mutable and stable in the Store. This doens't work properly though when component filters are changed, because this will cause the Store to dump all roots and re-initialize the tree (recreating the Element objects). So instead we key on Element ID (which is stable in this case) and use an LRU for eviction.
Brian Vaughn committed
Aug 23, 2021 at 19:22 UTC
edfe50510b4129295cbe49f05d7b94bbfc48da5b
4 files changed
+81
-17
packages/react-devtools-shared/src/__tests__/inspectedElement-test.js
+54
@@ -1895,6 +1895,60 @@ describe('InspectedElement', () => {
1895
`);
1896
});
1897
1898
+ // Regression test for github.com/facebook/react/issues/22099
1899
+ it('should not error when an unchanged component is re-inspected after component filters changed', async () => {
1900
+ const Example = () => <div />;
1901
+
1902
+ const container = document.createElement('div');
1903
+ await utils.actAsync(() => legacyRender(<Example />, container));
1904
+
1905
+ // Select/inspect element
1906
+ let inspectedElement = await inspectElementAtIndex(0);
1907
+ expect(inspectedElement).toMatchInlineSnapshot(`
1908
+ Object {
1909
+ "context": null,
1910
+ "events": undefined,
1911
+ "hooks": null,
1912
+ "id": 2,
1913
+ "owners": null,
1914
+ "props": Object {},
1915
+ "state": null,
1916
+ }
1917
+ `);
1918
+
1919
+ await utils.actAsync(async () => {
1920
+ // Ignore transient warning this causes
1921
+ utils.withErrorsOrWarningsIgnored(['No element found with id'], () => {
1922
+ store.componentFilters = [];
1923
+
1924
+ // Flush events to the renderer.
1925
+ jest.runOnlyPendingTimers();
1926
+ });
1927
+ }, false);
1928
+
1929
+ // HACK: Recreate TestRenderer instance because we rely on default state values
1930
+ // from props like defaultSelectedElementID and it's easier to reset here than
1931
+ // to read the TreeDispatcherContext and update the selected ID that way.
1932
+ // We're testing the inspected values here, not the context wiring, so that's ok.
1933
+ testRendererInstance = TestRenderer.create(null, {
1934
+ unstable_isConcurrent: true,
1935
+ });
1936
+
1937
+ // Select/inspect the same element again
1938
+ inspectedElement = await inspectElementAtIndex(0);
1939
+ expect(inspectedElement).toMatchInlineSnapshot(`
1940
+ Object {
1941
+ "context": null,
1942
+ "events": undefined,
1943
+ "hooks": null,
1944
+ "id": 2,
1945
+ "owners": null,
1946
+ "props": Object {},
1947
+ "state": null,
1948
+ }
1949
+ `);
1950
+ });
1951
+
1952
describe('$r', () => {
1953
it('should support function components', async () => {
1954
const Example = () => {
packages/react-devtools-shared/src/inspectedElementCache.js
+2
-3
@@ -88,7 +88,6 @@ export function inspectElement(
88
): InspectedElementFrontend | null {
89
const map = getRecordMap();
90
let record = map.get(element);
91
-
91
if (!record) {
92
const callbacks = new Set();
93
const wakeable: Wakeable = {
@@ -110,7 +109,7 @@ export function inspectElement(
109
if (rendererID == null) {
110
const rejectedRecord = ((newRecord: any): RejectedRecord);
111
rejectedRecord.status = Rejected;
113
- rejectedRecord.value = `Could not inspect element with id "${element.id}"`;
112
+ rejectedRecord.value = `Could not inspect element with id "${element.id}". No renderer found.`;
113
114
map.set(element, record);
115
@@ -134,7 +133,7 @@ export function inspectElement(
133
if (newRecord.status === Pending) {
134
const rejectedRecord = ((newRecord: any): RejectedRecord);
135
rejectedRecord.status = Rejected;
137
- rejectedRecord.value = `Could not inspect element with id "${element.id}"`;
136
+ rejectedRecord.value = `Could not inspect element with id "${element.id}". Error thrown:\n${error.message}`;
137
wake();
138
}
139
},
packages/react-devtools-shared/src/inspectedElementMutableSource.js
+24
-14
@@ -7,6 +7,7 @@
7
* @flow
8
*/
9
10
+import LRU from 'lru-cache';
11
import {
12
convertInspectedElementBackendToFrontend,
13
hydrateHelper,
@@ -14,6 +15,7 @@ import {
15
} from 'react-devtools-shared/src/backendAPI';
16
import {fillInPath} from 'react-devtools-shared/src/hydration';
17
18
+import type {LRUCache} from 'react-devtools-shared/src/types';
19
import type {FrontendBridge} from 'react-devtools-shared/src/bridge';
20
import type {
21
InspectElementFullData,
@@ -25,15 +27,21 @@ import type {
27
InspectedElementResponseType,
28
} from 'react-devtools-shared/src/devtools/views/Components/types';
29
28
-// Map an Element in the Store to the most recent copy of its inspected data.
29
-// As updates comes from the backend, inspected data is updated.
30
-// Both this map and the inspected objects in it are mutable.
31
-// They should never be read from directly during render;
32
-// Use a Suspense cache to ensure that transitions work correctly and there is no tearing.
33
-const inspectedElementMap: WeakMap<
34
- Element,
30
+// Maps element ID to inspected data.
31
+// We use an LRU for this rather than a WeakMap because of how the "no-change" optimization works.
32
+// When the frontend polls the backend for an update on the element that's currently inspected,
33
+// the backend will send a "no-change" message if the element hasn't updated (rendered) since the last time it was asked.
34
+// In thid case, the frontend cache should reuse the previous (cached) value.
35
+// Using a WeakMap keyed on Element generally works well for this, since Elements are mutable and stable in the Store.
36
+// This doens't work properly though when component filters are changed,
37
+// because this will cause the Store to dump all roots and re-initialize the tree (recreating the Element objects).
38
+// So instead we key on Element ID (which is stable in this case) and use an LRU for eviction.
39
+const inspectedElementCache: LRUCache<
40
+ number,
41
InspectedElementFrontend,
36
-> = new WeakMap();
42
+> = new LRU({
43
+ max: 25,
44
+});
45
46
type Path = Array<string | number>;
47
@@ -66,16 +74,18 @@ export function inspectElement({
74
switch (type) {
75
case 'no-change':
76
// This is a no-op for the purposes of our cache.
69
- inspectedElement = inspectedElementMap.get(element);
77
+ inspectedElement = inspectedElementCache.get(element.id);
78
if (inspectedElement != null) {
79
return [inspectedElement, type];
80
}
73
- break;
81
+
82
+ // We should only encounter this case in the event of a bug.
83
+ throw Error(`Cached data for element "${id}" not found`);
84
85
case 'not-found':
86
// This is effectively a no-op.
87
// If the Element is still in the Store, we can eagerly remove it from the Map.
78
- inspectedElementMap.delete(element);
88
+ inspectedElementCache.remove(element.id);
89
90
throw Error(`Element "${id}" not found`);
91
@@ -88,7 +98,7 @@ export function inspectElement({
98
fullData.value,
99
);
100
91
- inspectedElementMap.set(element, inspectedElement);
101
+ inspectedElementCache.set(element.id, inspectedElement);
102
103
return [inspectedElement, type];
104
@@ -98,7 +108,7 @@ export function inspectElement({
108
109
// A path has been hydrated.
110
// Merge it with the latest copy we have locally and resolve with the merged value.
101
- inspectedElement = inspectedElementMap.get(element) || null;
111
+ inspectedElement = inspectedElementCache.get(element.id) || null;
112
if (inspectedElement !== null) {
113
// Clone element
114
inspectedElement = {...inspectedElement};
@@ -111,7 +121,7 @@ export function inspectElement({
121
hydrateHelper(value, ((path: any): Path)),
122
);
123
114
- inspectedElementMap.set(element, inspectedElement);
124
+ inspectedElementCache.set(element.id, inspectedElement);
125
126
return [inspectedElement, type];
127
}
packages/react-devtools-shared/src/types.js
+1
@@ -87,6 +87,7 @@ export type HookNames = Map<HookSourceLocationKey, HookName>;
87
export type LRUCache<K, V> = {|
88
get: (key: K) => V,
89
has: (key: K) => boolean,
90
+ remove: (key: K) => void,
91
reset: () => void,
92
set: (key: K, value: V) => void,
93
|};