From fa50a4b6c3cbbd32a7e6d08ffd229b4d7fa84098 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 8 Jul 2021 13:37:07 -0400 Subject: [PATCH] DevTool: hook names cache no longer loses entries between selection 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. --- .../src/parseHookNames.js | 21 ++----- .../Components/InspectedElementContext.js | 20 ++++--- .../Components/InspectedElementHooksTree.js | 7 ++- .../src/hookNamesCache.js | 58 ++++++++++++------- packages/react-devtools-shared/src/types.js | 8 ++- 5 files changed, 67 insertions(+), 47 deletions(-) diff --git a/packages/react-devtools-extensions/src/parseHookNames.js b/packages/react-devtools-extensions/src/parseHookNames.js index bd4a7e63838..9336c5818ef 100644 --- 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 { @@ -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 index ebb26d0843e..6234aec4f1e 100644 --- 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( - 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 index 9712095d824..c4201ddeb4d 100644 --- 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 index 1b979c8e191..113ad64b860 100644 --- 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(record: Record): ResolvedRecord | RejectedRecord { } } -type HookNamesMap = WeakMap>; +// 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> = new WeakMap(); -function createMap(): HookNamesMap { - return new WeakMap(); -} - -function getRecordMap(): WeakMap> { - 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 index b9faeb7dbe9..92000f5f524 100644 --- 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; +// Map of hook source ("::") 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; export type LRUCache = {| get: (key: K) => V,