DevTool: hook names cache no longer loses entries between selection (#21831)
Made several changes to the hooks name cache to avoid losing cached data between selected elements: 1. No longer use React-managed cache. This had the unfortunate side effect of the inspected element cache also clearing the hook names cache. For now, instead, a module-level WeakMap cache is used. This isn't great but we can revisit it later. 2. Hooks are no longer the cache keys (since hook objects get recreated between element inspections). Instead a hook key string made of fileName + line number + column number is used. 3. If hook names have already been loaded for a component, skip showing the load button and just show the hook names by default when selecting the component.
Brian Vaughn committed
Jul 8, 2021 at 13:54 UTC
feb2f6892ab42a6efc1333bbf1544b7337e8f244
5 files changed
+67
-47
packages/react-devtools-extensions/src/parseHookNames.js
+6
-15
@@ -16,6 +16,7 @@ import {SourceMapConsumer} from 'source-map';
16
import {getHookName, isNonDeclarativePrimitiveHook} from './astUtils';
17
import {areSourceMapsAppliedToErrors} from './ErrorTester';
18
import {__DEBUG__} from 'react-devtools-shared/src/constants';
19
+import {getHookSourceLocationKey} from 'react-devtools-shared/src/hookNamesCache';
20
21
import type {
22
HooksNode,
@@ -101,17 +102,6 @@ const originalURLToMetadataCache: LRUCache<
102
},
103
});
104
104
-function getLocationKey({
105
- fileName,
106
- lineNumber,
107
- columnNumber,
108
-}: HookSource): string {
109
- if (fileName == null || lineNumber == null || columnNumber == null) {
110
- throw Error('Hook source code location not found.');
111
- }
112
- return `${fileName}:${lineNumber}:${columnNumber}`;
113
-}
114
-
105
export default async function parseHookNames(
106
hooksTree: HooksTree,
107
): Thenable<HookNames | null> {
@@ -138,9 +128,9 @@ export default async function parseHookNames(
128
throw Error('Hook source code location not found.');
129
}
130
141
- const locationKey = getLocationKey(hookSource);
131
+ const locationKey = getHookSourceLocationKey(hookSource);
132
if (!locationKeyToHookSourceData.has(locationKey)) {
143
- // Can't be null because getLocationKey() would have thrown
133
+ // Can't be null because getHookSourceLocationKey() would have thrown
134
const runtimeSourceURL = ((hookSource.fileName: any): string);
135
136
const hookSourceData: HookSourceData = {
@@ -373,7 +363,7 @@ function findHookNames(
363
return null; // Should not be reachable.
364
}
365
376
- const locationKey = getLocationKey(hookSource);
366
+ const locationKey = getHookSourceLocationKey(hookSource);
367
const hookSourceData = locationKeyToHookSourceData.get(locationKey);
368
if (!hookSourceData) {
369
return null; // Should not be reachable.
@@ -426,7 +416,8 @@ function findHookNames(
416
console.log(`findHookNames() Found name "${name || '-'}"`);
417
}
418
429
- map.set(hook, name);
419
+ const key = getHookSourceLocationKey(hookSource);
420
+ map.set(key, name);
421
});
422
423
return map;
packages/react-devtools-shared/src/devtools/views/Components/InspectedElementContext.js
+13
-7
@@ -25,7 +25,10 @@ import {
25
checkForUpdate,
26
inspectElement,
27
} from 'react-devtools-shared/src/inspectedElementCache';
28
-import {loadHookNames} from 'react-devtools-shared/src/hookNamesCache';
28
+import {
29
+ hasAlreadyLoadedHookNames,
30
+ loadHookNames,
31
+} from 'react-devtools-shared/src/hookNamesCache';
32
import LoadHookNamesFunctionContext from 'react-devtools-shared/src/devtools/views/Components/LoadHookNamesFunctionContext';
33
import {SettingsContext} from '../Settings/SettingsContext';
34
@@ -79,16 +82,19 @@ export function InspectedElementContextController({children}: Props) {
82
path: null,
83
});
84
85
+ const element =
86
+ selectedElementID !== null ? store.getElementByID(selectedElementID) : null;
87
+
88
+ const alreadyLoadedHookNames =
89
+ element != null && hasAlreadyLoadedHookNames(element);
90
+
91
// Parse the currently inspected element's hook names.
92
// This may be enabled by default (for all elements)
93
// or it may be opted into on a per-element basis (if it's too slow to be on by default).
94
const [parseHookNames, setParseHookNames] = useState<boolean>(
86
- parseHookNamesByDefault,
95
+ parseHookNamesByDefault || alreadyLoadedHookNames,
96
);
97
89
- const element =
90
- selectedElementID !== null ? store.getElementByID(selectedElementID) : null;
91
-
98
const elementHasChanged = element !== null && element !== state.element;
99
100
// Reset the cached inspected paths when a new element is selected.
@@ -98,7 +104,7 @@ export function InspectedElementContextController({children}: Props) {
104
path: null,
105
});
106
101
- setParseHookNames(parseHookNamesByDefault);
107
+ setParseHookNames(parseHookNamesByDefault || alreadyLoadedHookNames);
108
}
109
110
// Don't load a stale element from the backend; it wastes bridge bandwidth.
@@ -108,7 +114,7 @@ export function InspectedElementContextController({children}: Props) {
114
inspectedElement = inspectElement(element, state.path, store, bridge);
115
116
if (enableHookNameParsing) {
111
- if (parseHookNames) {
117
+ if (parseHookNames || alreadyLoadedHookNames) {
118
if (
119
inspectedElement !== null &&
120
inspectedElement.hooks !== null &&
packages/react-devtools-shared/src/devtools/views/Components/InspectedElementHooksTree.js
+6
-1
@@ -21,6 +21,7 @@ import Store from '../../store';
21
import styles from './InspectedElementHooksTree.css';
22
import useContextMenu from '../../ContextMenu/useContextMenu';
23
import {meta} from '../../../hydration';
24
+import {getHookSourceLocationKey} from 'react-devtools-shared/src/hookNamesCache';
25
import {
26
enableHookNameParsing,
27
enableProfilerChangedHookIndices,
@@ -235,7 +236,11 @@ function HookView({
236
let displayValue;
237
let isComplexDisplayValue = false;
238
238
- const hookName = hookNames != null ? hookNames.get(hook) : null;
239
+ const hookSource = hook.hookSource;
240
+ const hookName =
241
+ hookNames != null && hookSource != null
242
+ ? hookNames.get(getHookSourceLocationKey(hookSource))
243
+ : null;
244
const hookDisplayName = hookName ? (
245
<>
246
{name}
packages/react-devtools-shared/src/hookNamesCache.js
+37
-21
@@ -7,16 +7,19 @@
7
* @flow
8
*/
9
10
-import {unstable_getCacheForType as getCacheForType} from 'react';
10
import {enableHookNameParsing} from 'react-devtools-feature-flags';
11
import {__DEBUG__} from 'react-devtools-shared/src/constants';
12
13
import type {HooksTree} from 'react-debug-tools/src/ReactDebugHooks';
14
import type {Thenable, Wakeable} from 'shared/ReactTypes';
15
import type {Element} from './devtools/views/Components/types';
17
-import type {HookNames} from 'react-devtools-shared/src/types';
16
+import type {
17
+ HookNames,
18
+ HookSourceLocationKey,
19
+} from 'react-devtools-shared/src/types';
20
+import type {HookSource} from 'react-debug-tools/src/ReactDebugHooks';
21
19
-const TIMEOUT = 3000;
22
+const TIMEOUT = 5000;
23
24
const Pending = 0;
25
const Resolved = 1;
@@ -51,14 +54,15 @@ function readRecord<T>(record: Record<T>): ResolvedRecord<T> | RejectedRecord {
54
}
55
}
56
54
-type HookNamesMap = WeakMap<Element, Record<HookNames>>;
57
+// This is intentionally a module-level Map, rather than a React-managed one.
58
+// Otherwise, refreshing the inspected element cache would also clear this cache.
59
+// TODO Rethink this if the React API constraints change.
60
+// See https://github.com/reactwg/react-18/discussions/25#discussioncomment-980435
61
+const map: WeakMap<Element, Record<HookNames>> = new WeakMap();
62
56
-function createMap(): HookNamesMap {
57
- return new WeakMap();
58
-}
59
-
60
-function getRecordMap(): WeakMap<Element, Record<HookNames>> {
61
- return getCacheForType(createMap);
63
+export function hasAlreadyLoadedHookNames(element: Element): boolean {
64
+ const record = map.get(element);
65
+ return record != null && record.status === Resolved;
66
}
67
68
export function loadHookNames(
@@ -70,14 +74,15 @@ export function loadHookNames(
74
return null;
75
}
76
73
- const map = getRecordMap();
74
-
77
let record = map.get(element);
76
- if (record) {
77
- // TODO Do we need to update the Map to use new the hooks list objects as keys
78
- // or will these be stable between inspections as a component updates?
79
- // It seems like they're stable.
80
- } else {
78
+
79
+ if (__DEBUG__) {
80
+ console.groupCollapsed('loadHookNames() record:');
81
+ console.log(record);
82
+ console.groupEnd();
83
+ }
84
+
85
+ if (!record) {
86
const callbacks = new Set();
87
const wakeable: Wakeable = {
88
then(callback) {
@@ -126,14 +131,14 @@ export function loadHookNames(
131
wake();
132
},
133
function onError(error) {
129
- if (__DEBUG__) {
130
- console.log('[hookNamesCache] onError() error:', error);
131
- }
132
-
134
if (didTimeout) {
135
return;
136
}
137
138
+ if (__DEBUG__) {
139
+ console.log('[hookNamesCache] onError() error:', error);
140
+ }
141
+
142
const thrownRecord = ((newRecord: any): RejectedRecord);
143
thrownRecord.status = Rejected;
144
thrownRecord.value = null;
@@ -165,3 +170,14 @@ export function loadHookNames(
170
const response = readRecord(record).value;
171
return response;
172
}
173
+
174
+export function getHookSourceLocationKey({
175
+ fileName,
176
+ lineNumber,
177
+ columnNumber,
178
+}: HookSource): HookSourceLocationKey {
179
+ if (fileName == null || lineNumber == null || columnNumber == null) {
180
+ throw Error('Hook source code location not found.');
181
+ }
182
+ return `${fileName}:${lineNumber}:${columnNumber}`;
183
+}
packages/react-devtools-shared/src/types.js
+5
-3
@@ -7,8 +7,6 @@
7
* @flow
8
*/
9
10
-import type {HooksNode} from 'react-debug-tools/src/ReactDebugHooks';
11
-
10
export type Wall = {|
11
// `listen` returns the "unlisten" function.
12
listen: (fn: Function) => Function,
@@ -80,7 +78,11 @@ export type ComponentFilter =
78
| RegExpComponentFilter;
79
80
export type HookName = string | null;
83
-export type HookNames = Map<HooksNode, HookName>;
81
+// Map of hook source ("<filename>:<line-number>:<column-number>") to name.
82
+// Hook source is used instead of the hook itself becuase the latter is not stable between element inspections.
83
+// We use a Map rather than an Array because of nested hooks and traversal ordering.
84
+export type HookSourceLocationKey = string;
85
+export type HookNames = Map<HookSourceLocationKey, HookName>;
86
87
export type LRUCache<K, V> = {|
88
get: (key: K) => V,