DevTools: Replaced WeakMap with LRU for inspected element cache

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 = () => <div />;
+
+    const container = document.createElement('div');
+    await utils.actAsync(() => legacyRender(<Example />, 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 = () => {

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

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

@@ -7,13 +7,15 @@
  * @flow
  */
 
+import LRU from 'lru-cache';
 import {
   convertInspectedElementBackendToFrontend,
   hydrateHelper,
   inspectElement as inspectElementAPI,
 } 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<string | number>;
 
@@ -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];
         }

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

@@ -87,6 +87,7 @@ export type HookNames = Map<HookSourceLocationKey, HookName>;
 export type LRUCache<K, V> = {|
   get: (key: K) => V,
   has: (key: K) => boolean,
+  remove: (key: K) => void,
   reset: () => void,
   set: (key: K, value: V) => void,
 |};