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, |};