DevTools: Fixed potential cache miss when insepcting elements

If the DevTools UI is closed and reopened, the in-memory insepected element cache will be reset. The backend assumes this data is cached though, and may send a 'no-change' response if DevTools are re-opened and the element is re-inspected. To fix this, the frontend sends an explicit 'force' flag to tell the backend to always resend the full data.
facebook · Sep 30, 2021 · 1f1ae44 · 1f1ae44
1 parent 201af81
commit 1f1ae44
Show file tree

Hide file tree

Showing 8 changed files with 89 additions and 8 deletions.
diff --git a/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js
@@ -384,6 +384,67 @@ describe('InspectedElement', () => {
     `);
   });
 
+  // See github.com/facebook/react/issues/22241#issuecomment-931299972
+  it('should properly recover from a cache miss on the frontend', async () => {
+    let targetRenderCount = 0;
+
+    const Wrapper = ({children}) => children;
+    const Target = React.memo(props => {
+      targetRenderCount++;
+      // Even though his hook isn't referenced, it's used to observe backend rendering.
+      React.useState(0);
+      return null;
+    });
+
+    const container = document.createElement('div');
+    await utils.actAsync(() =>
+      legacyRender(
+        <Wrapper>
+          <Target a={1} b="abc" />
+        </Wrapper>,
+        container,
+      ),
+    );
+
+    targetRenderCount = 0;
+
+    let inspectedElement = await inspectElementAtIndex(1);
+    expect(targetRenderCount).toBe(1);
+    expect(inspectedElement.props).toMatchInlineSnapshot(`
+      Object {
+        "a": 1,
+        "b": "abc",
+      }
+    `);
+
+    const prevInspectedElement = inspectedElement;
+
+    // This test causes an intermediate error to be logged but we can ignore it.
+    console.error = () => {};
+
+    // Wait for our check-for-updates poll to get the new data.
+    jest.runOnlyPendingTimers();
+    await Promise.resolve();
+
+    // Clear the frontend cache to simulate DevTools being closed and re-opened.
+    // The backend still thinks the most recently-inspected element is still cached,
+    // so the frontend needs to tell it to resend a full value.
+    // We can verify this by asserting that the component is re-rendered again.
+    testRendererInstance = TestRenderer.create(null, {
+      unstable_isConcurrent: true,
+    });
+
+    const {
+      clearCacheForTests,
+    } = require('react-devtools-shared/src/inspectedElementMutableSource');
+    clearCacheForTests();
+
+    targetRenderCount = 0;
+    inspectedElement = await inspectElementAtIndex(1);
+    // TODO expect(targetRenderCount).toBe(1);
+    expect(inspectedElement).toEqual(prevInspectedElement);
+  });
+
   it('should temporarily disable console logging when re-running a component to inspect its hooks', async () => {
     let targetRenderCount = 0;
 

diff --git a/packages/react-devtools-shared/src/backend/agent.js b/packages/react-devtools-shared/src/backend/agent.js
@@ -72,6 +72,7 @@ type CopyElementParams = {|
 |};
 
 type InspectElementParams = {|
+  forceFullData: boolean,
   id: number,
   path: Array<string | number> | null,
   rendererID: number,
@@ -346,6 +347,7 @@ export default class Agent extends EventEmitter<{|
   };
 
   inspectElement = ({
+    forceFullData,
     id,
     path,
     rendererID,
@@ -357,7 +359,7 @@ export default class Agent extends EventEmitter<{|
     } else {
       this._bridge.send(
         'inspectedElement',
-        renderer.inspectElement(requestID, id, path),
+        renderer.inspectElement(requestID, id, path, forceFullData),
       );
 
       // When user selects an element, stop trying to restore the selection,

diff --git a/packages/react-devtools-shared/src/backend/legacy/renderer.js b/packages/react-devtools-shared/src/backend/legacy/renderer.js
@@ -694,8 +694,9 @@ export function attach(
     requestID: number,
     id: number,
     path: Array<string | number> | null,
+    forceFullData: boolean,
   ): InspectedElementPayload {
-    if (currentlyInspectedElementID !== id) {
+    if (forceFullData || currentlyInspectedElementID !== id) {
       currentlyInspectedElementID = id;
       currentlyInspectedPaths = {};
     }

diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js
@@ -3439,12 +3439,13 @@ export function attach(
     requestID: number,
     id: number,
     path: Array<string | number> | null,
+    forceFullData: boolean,
   ): InspectedElementPayload {
     if (path !== null) {
       mergeInspectedPaths(path);
     }
 
-    if (isMostRecentlyInspectedElement(id)) {
+    if (isMostRecentlyInspectedElement(id) && !forceFullData) {
       if (!hasElementUpdatedSinceLastInspected) {
         if (path !== null) {
           let secondaryCategory = null;

diff --git a/packages/react-devtools-shared/src/backend/types.js b/packages/react-devtools-shared/src/backend/types.js
@@ -339,6 +339,7 @@ export type RendererInterface = {
     requestID: number,
     id: number,
     inspectedPaths: Object,
+    forceFullData: boolean,
   ) => InspectedElementPayload,
   logElementToConsole: (id: number) => void,
   overrideError: (id: number, forceError: boolean) => void,

diff --git a/packages/react-devtools-shared/src/backendAPI.js b/packages/react-devtools-shared/src/backendAPI.js
@@ -86,11 +86,13 @@ export function copyInspectedElementPath({
 
 export function inspectElement({
   bridge,
+  forceFullData,
   id,
   path,
   rendererID,
 }: {|
   bridge: FrontendBridge,
+  forceFullData: boolean,
   id: number,
   path: Array<string | number> | null,
   rendererID: number,
@@ -103,6 +105,7 @@ export function inspectElement({
   );
 
   bridge.send('inspectElement', {
+    forceFullData,
     id,
     path,
     rendererID,

diff --git a/packages/react-devtools-shared/src/bridge.js b/packages/react-devtools-shared/src/bridge.js
@@ -138,6 +138,7 @@ type ViewAttributeSourceParams = {|
 
 type InspectElementParams = {|
   ...ElementAndRendererID,
+  forceFullData: boolean,
   path: Array<number | string> | null,
   requestID: number,
 |};

diff --git a/packages/react-devtools-shared/src/inspectedElementMutableSource.js b/packages/react-devtools-shared/src/inspectedElementMutableSource.js
@@ -62,8 +62,15 @@ export function inspectElement({
   rendererID: number,
 |}): Promise<InspectElementReturnType> {
   const {id} = element;
+
+  // This could indicate that the DevTools UI has been closed and reopened.
+  // The in-memory cache will be clear but the backend still thinks we have cached data.
+  // In this case, we need to tell it to resend the full data.
+  const forceFullData = !inspectedElementCache.has(id);
+
   return inspectElementAPI({
     bridge,
+    forceFullData,
     id,
     path,
     rendererID,
@@ -74,7 +81,7 @@ export function inspectElement({
     switch (type) {
       case 'no-change':
         // This is a no-op for the purposes of our cache.
-        inspectedElement = inspectedElementCache.get(element.id);
+        inspectedElement = inspectedElementCache.get(id);
         if (inspectedElement != null) {
           return [inspectedElement, type];
         }
@@ -85,7 +92,7 @@ export function inspectElement({
       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.
-        inspectedElementCache.remove(element.id);
+        inspectedElementCache.remove(id);
 
         throw Error(`Element "${id}" not found`);
 
@@ -98,7 +105,7 @@ export function inspectElement({
           fullData.value,
         );
 
-        inspectedElementCache.set(element.id, inspectedElement);
+        inspectedElementCache.set(id, inspectedElement);
 
         return [inspectedElement, type];
 
@@ -108,7 +115,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 = inspectedElementCache.get(element.id) || null;
+        inspectedElement = inspectedElementCache.get(id) || null;
         if (inspectedElement !== null) {
           // Clone element
           inspectedElement = {...inspectedElement};
@@ -121,7 +128,7 @@ export function inspectElement({
             hydrateHelper(value, ((path: any): Path)),
           );
 
-          inspectedElementCache.set(element.id, inspectedElement);
+          inspectedElementCache.set(id, inspectedElement);
 
           return [inspectedElement, type];
         }
@@ -140,3 +147,7 @@ export function inspectElement({
     throw Error(`Unable to inspect element with id "${id}"`);
   });
 }
+
+export function clearCacheForTests(): void {
+  inspectedElementCache.reset();
+}