From aff049b0d6f7d915797f6d8793d684ce95341783 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 23 Aug 2021 15:59:43 -0700 Subject: [PATCH] DevTools: Replaced WeakMap with LRU for inspected element cache 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. --- .../src/__tests__/inspectedElement-test.js | 54 +++++++++++++++++++ .../src/inspectedElementCache.js | 5 +- .../src/inspectedElementMutableSource.js | 38 ++++++++----- packages/react-devtools-shared/src/types.js | 1 + 4 files changed, 81 insertions(+), 17 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js index 886a54ceeec1b..4586e34e32e13 100644 --- a/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js +++ b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js @@ -1895,6 +1895,60 @@ describe('InspectedElement', () => { `); }); + // Regression test for github.com/facebook/react/issues/22099 + it('should not error when an unchanged component is re-inspected after component filters changed', async () => { + const Example = () =>
; + + const container = document.createElement('div'); + await utils.actAsync(() => legacyRender(, container)); + + // Select/inspect element + let inspectedElement = await inspectElementAtIndex(0); + expect(inspectedElement).toMatchInlineSnapshot(` + Object { + "context": null, + "events": undefined, + "hooks": null, + "id": 2, + "owners": null, + "props": Object {}, + "state": null, + } + `); + + await utils.actAsync(async () => { + // Ignore transient warning this causes + utils.withErrorsOrWarningsIgnored(['No element found with id'], () => { + store.componentFilters = []; + + // Flush events to the renderer. + jest.runOnlyPendingTimers(); + }); + }, false); + + // HACK: Recreate TestRenderer instance because we rely on default state values + // from props like defaultSelectedElementID and it's easier to reset here than + // to read the TreeDispatcherContext and update the selected ID that way. + // We're testing the inspected values here, not the context wiring, so that's ok. + testRendererInstance = TestRenderer.create(null, { + unstable_isConcurrent: true, + }); + + // Select/inspect the same element again + inspectedElement = await inspectElementAtIndex(0); + expect(inspectedElement).toMatchInlineSnapshot(` + Object { + "context": null, + "events": undefined, + "hooks": null, + "id": 2, + "owners": null, + "props": Object {}, + "state": null, + } + `); + }); + describe('$r', () => { it('should support function components', async () => { const Example = () => { diff --git a/packages/react-devtools-shared/src/inspectedElementCache.js b/packages/react-devtools-shared/src/inspectedElementCache.js index f5d1ba5b9884d..820a2a59a18b7 100644 --- a/packages/react-devtools-shared/src/inspectedElementCache.js +++ b/packages/react-devtools-shared/src/inspectedElementCache.js @@ -88,7 +88,6 @@ export function inspectElement( ): InspectedElementFrontend | null { const map = getRecordMap(); let record = map.get(element); - if (!record) { const callbacks = new Set(); const wakeable: Wakeable = { @@ -110,7 +109,7 @@ export function inspectElement( if (rendererID == null) { const rejectedRecord = ((newRecord: any): RejectedRecord); rejectedRecord.status = Rejected; - rejectedRecord.value = `Could not inspect element with id "${element.id}"`; + rejectedRecord.value = `Could not inspect element with id "${element.id}". No renderer found.`; map.set(element, record); @@ -134,7 +133,7 @@ export function inspectElement( if (newRecord.status === Pending) { const rejectedRecord = ((newRecord: any): RejectedRecord); rejectedRecord.status = Rejected; - rejectedRecord.value = `Could not inspect element with id "${element.id}"`; + rejectedRecord.value = `Could not inspect element with id "${element.id}". Error thrown:\n${error.message}`; wake(); } }, diff --git a/packages/react-devtools-shared/src/inspectedElementMutableSource.js b/packages/react-devtools-shared/src/inspectedElementMutableSource.js index f53ab6398b08d..d0c2880005950 100644 --- a/packages/react-devtools-shared/src/inspectedElementMutableSource.js +++ b/packages/react-devtools-shared/src/inspectedElementMutableSource.js @@ -7,6 +7,7 @@ * @flow */ +import LRU from 'lru-cache'; import { convertInspectedElementBackendToFrontend, hydrateHelper, @@ -14,6 +15,7 @@ import { } from 'react-devtools-shared/src/backendAPI'; import {fillInPath} from 'react-devtools-shared/src/hydration'; +import type {LRUCache} from 'react-devtools-shared/src/types'; import type {FrontendBridge} from 'react-devtools-shared/src/bridge'; import type { InspectElementFullData, @@ -25,15 +27,21 @@ import type { InspectedElementResponseType, } from 'react-devtools-shared/src/devtools/views/Components/types'; -// Map an Element in the Store to the most recent copy of its inspected data. -// As updates comes from the backend, inspected data is updated. -// Both this map and the inspected objects in it are mutable. -// They should never be read from directly during render; -// Use a Suspense cache to ensure that transitions work correctly and there is no tearing. -const inspectedElementMap: WeakMap< - Element, +// Maps element ID to inspected data. +// 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. +const inspectedElementCache: LRUCache< + number, InspectedElementFrontend, -> = new WeakMap(); +> = new LRU({ + max: 25, +}); type Path = Array; @@ -66,16 +74,18 @@ export function inspectElement({ switch (type) { case 'no-change': // This is a no-op for the purposes of our cache. - inspectedElement = inspectedElementMap.get(element); + inspectedElement = inspectedElementCache.get(element.id); if (inspectedElement != null) { return [inspectedElement, type]; } - break; + + // We should only encounter this case in the event of a bug. + throw Error(`Cached data for element "${id}" not found`); case 'not-found': // This is effectively a no-op. // If the Element is still in the Store, we can eagerly remove it from the Map. - inspectedElementMap.delete(element); + inspectedElementCache.remove(element.id); throw Error(`Element "${id}" not found`); @@ -88,7 +98,7 @@ export function inspectElement({ fullData.value, ); - inspectedElementMap.set(element, inspectedElement); + inspectedElementCache.set(element.id, inspectedElement); return [inspectedElement, type]; @@ -98,7 +108,7 @@ export function inspectElement({ // A path has been hydrated. // Merge it with the latest copy we have locally and resolve with the merged value. - inspectedElement = inspectedElementMap.get(element) || null; + inspectedElement = inspectedElementCache.get(element.id) || null; if (inspectedElement !== null) { // Clone element inspectedElement = {...inspectedElement}; @@ -111,7 +121,7 @@ export function inspectElement({ hydrateHelper(value, ((path: any): Path)), ); - inspectedElementMap.set(element, inspectedElement); + inspectedElementCache.set(element.id, inspectedElement); return [inspectedElement, type]; } diff --git a/packages/react-devtools-shared/src/types.js b/packages/react-devtools-shared/src/types.js index 695dc95163fde..fbde156a5c103 100644 --- a/packages/react-devtools-shared/src/types.js +++ b/packages/react-devtools-shared/src/types.js @@ -87,6 +87,7 @@ export type HookNames = Map; export type LRUCache = {| get: (key: K) => V, has: (key: K) => boolean, + remove: (key: K) => void, reset: () => void, set: (key: K, value: V) => void, |};