DevTool: hook names cache no longer loses entries between selection

packages/react-devtools-extensions/src/parseHookNames.js

@@ -16,6 +16,7 @@ import {SourceMapConsumer} from 'source-map';
 import {getHookName, isNonDeclarativePrimitiveHook} from './astUtils';
 import {areSourceMapsAppliedToErrors} from './ErrorTester';
 import {__DEBUG__} from 'react-devtools-shared/src/constants';
+import {getHookSourceLocationKey} from 'react-devtools-shared/src/hookNamesCache';
 
 import type {
   HooksNode,
@@ -101,17 +102,6 @@ const originalURLToMetadataCache: LRUCache<
   },
 });
 
-function getLocationKey({
-  fileName,
-  lineNumber,
-  columnNumber,
-}: HookSource): string {
-  if (fileName == null || lineNumber == null || columnNumber == null) {
-    throw Error('Hook source code location not found.');
-  }
-  return `${fileName}:${lineNumber}:${columnNumber}`;
-}
-
 export default async function parseHookNames(
   hooksTree: HooksTree,
 ): Thenable<HookNames | null> {
@@ -138,9 +128,9 @@ export default async function parseHookNames(
       throw Error('Hook source code location not found.');
     }
 
-    const locationKey = getLocationKey(hookSource);
+    const locationKey = getHookSourceLocationKey(hookSource);
     if (!locationKeyToHookSourceData.has(locationKey)) {
-      // Can't be null because getLocationKey() would have thrown
+      // Can't be null because getHookSourceLocationKey() would have thrown
       const runtimeSourceURL = ((hookSource.fileName: any): string);
 
       const hookSourceData: HookSourceData = {
@@ -373,7 +363,7 @@ function findHookNames(
       return null; // Should not be reachable.
     }
 
-    const locationKey = getLocationKey(hookSource);
+    const locationKey = getHookSourceLocationKey(hookSource);
     const hookSourceData = locationKeyToHookSourceData.get(locationKey);
     if (!hookSourceData) {
       return null; // Should not be reachable.
@@ -426,7 +416,8 @@ function findHookNames(
       console.log(`findHookNames() Found name "${name || '-'}"`);
     }
 
-    map.set(hook, name);
+    const key = getHookSourceLocationKey(hookSource);
+    map.set(key, name);
   });
 
   return map;

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

@@ -25,7 +25,10 @@ import {
   checkForUpdate,
   inspectElement,
 } from 'react-devtools-shared/src/inspectedElementCache';
-import {loadHookNames} from 'react-devtools-shared/src/hookNamesCache';
+import {
+  hasAlreadyLoadedHookNames,
+  loadHookNames,
+} from 'react-devtools-shared/src/hookNamesCache';
 import LoadHookNamesFunctionContext from 'react-devtools-shared/src/devtools/views/Components/LoadHookNamesFunctionContext';
 import {SettingsContext} from '../Settings/SettingsContext';
 
@@ -79,16 +82,19 @@ export function InspectedElementContextController({children}: Props) {
     path: null,
   });
 
+  const element =
+    selectedElementID !== null ? store.getElementByID(selectedElementID) : null;
+
+  const alreadyLoadedHookNames =
+    element != null && hasAlreadyLoadedHookNames(element);
+
   // Parse the currently inspected element's hook names.
   // This may be enabled by default (for all elements)
   // or it may be opted into on a per-element basis (if it's too slow to be on by default).
   const [parseHookNames, setParseHookNames] = useState<boolean>(
-    parseHookNamesByDefault,
+    parseHookNamesByDefault || alreadyLoadedHookNames,
   );
 
-  const element =
-    selectedElementID !== null ? store.getElementByID(selectedElementID) : null;
-
   const elementHasChanged = element !== null && element !== state.element;
 
   // Reset the cached inspected paths when a new element is selected.
@@ -98,7 +104,7 @@ export function InspectedElementContextController({children}: Props) {
       path: null,
     });
 
-    setParseHookNames(parseHookNamesByDefault);
+    setParseHookNames(parseHookNamesByDefault || alreadyLoadedHookNames);
   }
 
   // Don't load a stale element from the backend; it wastes bridge bandwidth.
@@ -108,7 +114,7 @@ export function InspectedElementContextController({children}: Props) {
     inspectedElement = inspectElement(element, state.path, store, bridge);
 
     if (enableHookNameParsing) {
-      if (parseHookNames) {
+      if (parseHookNames || alreadyLoadedHookNames) {
         if (
           inspectedElement !== null &&
           inspectedElement.hooks !== null &&

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

@@ -21,6 +21,7 @@ import Store from '../../store';
 import styles from './InspectedElementHooksTree.css';
 import useContextMenu from '../../ContextMenu/useContextMenu';
 import {meta} from '../../../hydration';
+import {getHookSourceLocationKey} from 'react-devtools-shared/src/hookNamesCache';
 import {
   enableHookNameParsing,
   enableProfilerChangedHookIndices,
@@ -235,7 +236,11 @@ function HookView({
   let displayValue;
   let isComplexDisplayValue = false;
 
-  const hookName = hookNames != null ? hookNames.get(hook) : null;
+  const hookSource = hook.hookSource;
+  const hookName =
+    hookNames != null && hookSource != null
+      ? hookNames.get(getHookSourceLocationKey(hookSource))
+      : null;
   const hookDisplayName = hookName ? (
     <>
       {name}

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

@@ -7,16 +7,19 @@
  * @flow
  */
 
-import {unstable_getCacheForType as getCacheForType} from 'react';
 import {enableHookNameParsing} from 'react-devtools-feature-flags';
 import {__DEBUG__} from 'react-devtools-shared/src/constants';
 
 import type {HooksTree} from 'react-debug-tools/src/ReactDebugHooks';
 import type {Thenable, Wakeable} from 'shared/ReactTypes';
 import type {Element} from './devtools/views/Components/types';
-import type {HookNames} from 'react-devtools-shared/src/types';
+import type {
+  HookNames,
+  HookSourceLocationKey,
+} from 'react-devtools-shared/src/types';
+import type {HookSource} from 'react-debug-tools/src/ReactDebugHooks';
 
-const TIMEOUT = 3000;
+const TIMEOUT = 5000;
 
 const Pending = 0;
 const Resolved = 1;
@@ -51,14 +54,15 @@ function readRecord<T>(record: Record<T>): ResolvedRecord<T> | RejectedRecord {
   }
 }
 
-type HookNamesMap = WeakMap<Element, Record<HookNames>>;
+// This is intentionally a module-level Map, rather than a React-managed one.
+// Otherwise, refreshing the inspected element cache would also clear this cache.
+// TODO Rethink this if the React API constraints change.
+// See https://github.com/reactwg/react-18/discussions/25#discussioncomment-980435
+const map: WeakMap<Element, Record<HookNames>> = new WeakMap();
 
-function createMap(): HookNamesMap {
-  return new WeakMap();
-}
-
-function getRecordMap(): WeakMap<Element, Record<HookNames>> {
-  return getCacheForType(createMap);
+export function hasAlreadyLoadedHookNames(element: Element): boolean {
+  const record = map.get(element);
+  return record != null && record.status === Resolved;
 }
 
 export function loadHookNames(
@@ -70,14 +74,15 @@ export function loadHookNames(
     return null;
   }
 
-  const map = getRecordMap();
-
   let record = map.get(element);
-  if (record) {
-    // TODO Do we need to update the Map to use new the hooks list objects as keys
-    // or will these be stable between inspections as a component updates?
-    // It seems like they're stable.
-  } else {
+
+  if (__DEBUG__) {
+    console.groupCollapsed('loadHookNames() record:');
+    console.log(record);
+    console.groupEnd();
+  }
+
+  if (!record) {
     const callbacks = new Set();
     const wakeable: Wakeable = {
       then(callback) {
@@ -126,14 +131,14 @@ export function loadHookNames(
         wake();
       },
       function onError(error) {
-        if (__DEBUG__) {
-          console.log('[hookNamesCache] onError() error:', error);
-        }
-
         if (didTimeout) {
           return;
         }
 
+        if (__DEBUG__) {
+          console.log('[hookNamesCache] onError() error:', error);
+        }
+
         const thrownRecord = ((newRecord: any): RejectedRecord);
         thrownRecord.status = Rejected;
         thrownRecord.value = null;
@@ -165,3 +170,14 @@ export function loadHookNames(
   const response = readRecord(record).value;
   return response;
 }
+
+export function getHookSourceLocationKey({
+  fileName,
+  lineNumber,
+  columnNumber,
+}: HookSource): HookSourceLocationKey {
+  if (fileName == null || lineNumber == null || columnNumber == null) {
+    throw Error('Hook source code location not found.');
+  }
+  return `${fileName}:${lineNumber}:${columnNumber}`;
+}

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

@@ -7,8 +7,6 @@
  * @flow
  */
 
-import type {HooksNode} from 'react-debug-tools/src/ReactDebugHooks';
-
 export type Wall = {|
   // `listen` returns the "unlisten" function.
   listen: (fn: Function) => Function,
@@ -80,7 +78,11 @@ export type ComponentFilter =
   | RegExpComponentFilter;
 
 export type HookName = string | null;
-export type HookNames = Map<HooksNode, HookName>;
+// Map of hook source ("<filename>:<line-number>:<column-number>") to name.
+// Hook source is used instead of the hook itself becuase the latter is not stable between element inspections.
+// We use a Map rather than an Array because of nested hooks and traversal ordering.
+export type HookSourceLocationKey = string;
+export type HookNames = Map<HookSourceLocationKey, HookName>;
 
 export type LRUCache<K, V> = {|
   get: (key: K) => V,