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.
facebook · Jul 8, 2021 · feb2f68 · feb2f68
1 parent 241485a
commit feb2f68
Show file tree

Hide file tree

Showing 5 changed files with 67 additions and 47 deletions.
diff --git a/packages/react-devtools-extensions/src/parseHookNames.js b/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;

diff --git a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementContext.js b/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 &&

diff --git a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementHooksTree.js b/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}

diff --git a/packages/react-devtools-shared/src/hookNamesCache.js b/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}`;
+}
diff --git a/packages/react-devtools-shared/src/types.js b/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,