DevTools: Replaced WeakMap with LRU for inspected element cache (face…

…book#22160)

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.

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