[DevTools] Further Refactoring of Unmounts (#30658)

Stacked on #30625 and #30657. This ensures that we only create instances during the commit reconciliation and that we don't create unnecessary instances for things that are filtered or not mounted. This ensures that we also can rely on the reconciliation to do all the clean up. Now everything is created and deleted as a pair in the same pass. Previously we were including unfiltered components in the owner stack which probably doesn't make sense since you're intending to filter them everywhere presumably. However, it also means that those links were broken since you can't link into owners that don't exist in the parent tree. The main complication is the component filters. It relied on not unmounting the old instances. I had to update some tests that asserted on ids that are now shifted. For warnings/errors tracking I now restore them back into the pending set when they unmount. Basically it puts them back into their "pre-commit" state. That way when they remount they’re still there. For restoring the current selection I use the tracked path mechanism instead of relying on the id being unchanged. This is better anyway because if you filter out the currently selected item it's better to select the nearest match instead of just losing the selection.
facebook · Aug 12, 2024 · 25c584f · 25c584f
1 parent b4c3801
commit 25c584f
Show file tree

Hide file tree

Showing 6 changed files with 295 additions and 226 deletions.
diff --git a/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js
@@ -34,6 +34,8 @@ describe('InspectedElement', () => {
   let SettingsContextController;
   let StoreContext;
   let TreeContextController;
+  let TreeStateContext;
+  let TreeDispatcherContext;
 
   let TestUtilsAct;
   let TestRendererAct;
@@ -73,6 +75,10 @@ describe('InspectedElement', () => {
       require('react-devtools-shared/src/devtools/views/context').StoreContext;
     TreeContextController =
       require('react-devtools-shared/src/devtools/views/Components/TreeContext').TreeContextController;
+    TreeStateContext =
+      require('react-devtools-shared/src/devtools/views/Components/TreeContext').TreeStateContext;
+    TreeDispatcherContext =
+      require('react-devtools-shared/src/devtools/views/Components/TreeContext').TreeDispatcherContext;
 
     // Used by inspectElementAtIndex() helper function
     utils.act(() => {
@@ -2142,7 +2148,7 @@ describe('InspectedElement', () => {
         "context": null,
         "events": undefined,
         "hooks": null,
-        "id": 2,
+        "id": 4,
         "owners": null,
         "props": {},
         "rootType": "createRoot()",
@@ -2893,15 +2899,15 @@ describe('InspectedElement', () => {
           "compiledWithForget": false,
           "displayName": "Child",
           "hocDisplayNames": null,
-          "id": 5,
+          "id": 8,
           "key": null,
           "type": 5,
         },
         {
           "compiledWithForget": false,
           "displayName": "App",
           "hocDisplayNames": null,
-          "id": 4,
+          "id": 7,
           "key": null,
           "type": 5,
         },
@@ -3016,4 +3022,132 @@ describe('InspectedElement', () => {
       );
     });
   });
+
+  it('should properly handle when components filters are updated', async () => {
+    const Wrapper = ({children}) => children;
+
+    let state;
+    let dispatch;
+    const Capture = () => {
+      dispatch = React.useContext(TreeDispatcherContext);
+      state = React.useContext(TreeStateContext);
+      return null;
+    };
+
+    function Child({logError = false, logWarning = false}) {
+      if (logError === true) {
+        console.error('test-only: error');
+      }
+      if (logWarning === true) {
+        console.warn('test-only: warning');
+      }
+      return null;
+    }
+
+    async function selectNextErrorOrWarning() {
+      await utils.actAsync(
+        () =>
+          dispatch({type: 'SELECT_NEXT_ELEMENT_WITH_ERROR_OR_WARNING_IN_TREE'}),
+        false,
+      );
+    }
+
+    async function selectPreviousErrorOrWarning() {
+      await utils.actAsync(
+        () =>
+          dispatch({
+            type: 'SELECT_PREVIOUS_ELEMENT_WITH_ERROR_OR_WARNING_IN_TREE',
+          }),
+        false,
+      );
+    }
+
+    withErrorsOrWarningsIgnored(['test-only:'], () =>
+      utils.act(() =>
+        render(
+          <React.Fragment>
+            <Wrapper>
+              <Child logWarning={true} />
+            </Wrapper>
+            <Wrapper>
+              <Wrapper>
+                <Child logWarning={true} />
+              </Wrapper>
+            </Wrapper>
+          </React.Fragment>,
+        ),
+      ),
+    );
+
+    utils.act(() =>
+      TestRenderer.create(
+        <Contexts>
+          <Capture />
+        </Contexts>,
+      ),
+    );
+    expect(state).toMatchInlineSnapshot(`
+      ✕ 0, ⚠ 2
+      [root]
+         ▾ <Wrapper>
+             <Child> ⚠
+         ▾ <Wrapper>
+           ▾ <Wrapper>
+               <Child> ⚠
+    `);
+
+    await selectNextErrorOrWarning();
+    expect(state).toMatchInlineSnapshot(`
+      ✕ 0, ⚠ 2
+      [root]
+         ▾ <Wrapper>
+      →      <Child> ⚠
+         ▾ <Wrapper>
+           ▾ <Wrapper>
+               <Child> ⚠
+    `);
+
+    await utils.actAsync(() => {
+      store.componentFilters = [utils.createDisplayNameFilter('Wrapper')];
+    }, false);
+
+    expect(state).toMatchInlineSnapshot(`
+      ✕ 0, ⚠ 2
+      [root]
+      →    <Child> ⚠
+           <Child> ⚠
+    `);
+
+    await selectNextErrorOrWarning();
+    expect(state).toMatchInlineSnapshot(`
+      ✕ 0, ⚠ 2
+      [root]
+           <Child> ⚠
+      →    <Child> ⚠
+    `);
+
+    await utils.actAsync(() => {
+      store.componentFilters = [];
+    }, false);
+    expect(state).toMatchInlineSnapshot(`
+      ✕ 0, ⚠ 2
+      [root]
+         ▾ <Wrapper>
+             <Child> ⚠
+         ▾ <Wrapper>
+           ▾ <Wrapper>
+      →        <Child> ⚠
+    `);
+
+    await selectPreviousErrorOrWarning();
+    expect(state).toMatchInlineSnapshot(`
+      ✕ 0, ⚠ 2
+      [root]
+         ▾ <Wrapper>
+      →      <Child> ⚠
+         ▾ <Wrapper>
+           ▾ <Wrapper>
+               <Child> ⚠
+    `);
+  });
 });
diff --git a/packages/react-devtools-shared/src/__tests__/ownersListContext-test.js b/packages/react-devtools-shared/src/__tests__/ownersListContext-test.js
@@ -156,7 +156,6 @@ describe('OwnersListContext', () => {
     expect(await getOwnersListForOwner(firstChild)).toMatchInlineSnapshot(`
       [
         "Grandparent",
-        "Parent",
         "Child",
       ]
     `);

diff --git a/packages/react-devtools-shared/src/__tests__/profilingCache-test.js b/packages/react-devtools-shared/src/__tests__/profilingCache-test.js
@@ -1251,17 +1251,17 @@ describe('ProfilingCache', () => {
       Map {
         1 => 16,
         2 => 16,
+        3 => 1,
         4 => 1,
-        6 => 1,
       }
     `);
 
     expect(commitData[0].fiberSelfDurations).toMatchInlineSnapshot(`
       Map {
         1 => 0,
         2 => 10,
+        3 => 1,
         4 => 1,
-        6 => 1,
       }
     `);
   });
@@ -1322,13 +1322,13 @@ describe('ProfilingCache', () => {
     `);
     expect(commitData[1].fiberActualDurations).toMatchInlineSnapshot(`
       Map {
-        7 => 3,
+        5 => 3,
         3 => 3,
       }
     `);
     expect(commitData[1].fiberSelfDurations).toMatchInlineSnapshot(`
       Map {
-        7 => 3,
+        5 => 3,
         3 => 0,
       }
     `);

diff --git a/packages/react-devtools-shared/src/__tests__/treeContext-test.js b/packages/react-devtools-shared/src/__tests__/treeContext-test.js
@@ -2286,91 +2286,6 @@ describe('TreeListContext', () => {
       `);
     });
 
-    it('should properly handle when components filters are updated', () => {
-      const Wrapper = ({children}) => children;
-
-      withErrorsOrWarningsIgnored(['test-only:'], () =>
-        utils.act(() =>
-          render(
-            <React.Fragment>
-              <Wrapper>
-                <Child logWarning={true} />
-              </Wrapper>
-              <Wrapper>
-                <Wrapper>
-                  <Child logWarning={true} />
-                </Wrapper>
-              </Wrapper>
-            </React.Fragment>,
-          ),
-        ),
-      );
-
-      utils.act(() => TestRenderer.create(<Contexts />));
-      expect(state).toMatchInlineSnapshot(`
-        ✕ 0, ⚠ 2
-        [root]
-           ▾ <Wrapper>
-               <Child> ⚠
-           ▾ <Wrapper>
-             ▾ <Wrapper>
-                 <Child> ⚠
-      `);
-
-      selectNextErrorOrWarning();
-      expect(state).toMatchInlineSnapshot(`
-        ✕ 0, ⚠ 2
-        [root]
-           ▾ <Wrapper>
-        →      <Child> ⚠
-           ▾ <Wrapper>
-             ▾ <Wrapper>
-                 <Child> ⚠
-      `);
-
-      utils.act(() => {
-        store.componentFilters = [utils.createDisplayNameFilter('Wrapper')];
-      });
-      expect(state).toMatchInlineSnapshot(`
-        ✕ 0, ⚠ 2
-        [root]
-        →    <Child> ⚠
-             <Child> ⚠
-      `);
-
-      selectNextErrorOrWarning();
-      expect(state).toMatchInlineSnapshot(`
-        ✕ 0, ⚠ 2
-        [root]
-             <Child> ⚠
-        →    <Child> ⚠
-      `);
-
-      utils.act(() => {
-        store.componentFilters = [];
-      });
-      expect(state).toMatchInlineSnapshot(`
-        ✕ 0, ⚠ 2
-        [root]
-           ▾ <Wrapper>
-               <Child> ⚠
-           ▾ <Wrapper>
-             ▾ <Wrapper>
-        →        <Child> ⚠
-      `);
-
-      selectPreviousErrorOrWarning();
-      expect(state).toMatchInlineSnapshot(`
-        ✕ 0, ⚠ 2
-        [root]
-           ▾ <Wrapper>
-        →      <Child> ⚠
-           ▾ <Wrapper>
-             ▾ <Wrapper>
-                 <Child> ⚠
-      `);
-    });
-
     it('should preserve errors for fibers even if they are filtered out of the tree initially', () => {
       const Wrapper = ({children}) => children;
 

diff --git a/packages/react-devtools-shared/src/backend/agent.js b/packages/react-devtools-shared/src/backend/agent.js
@@ -795,10 +795,23 @@ export default class Agent extends EventEmitter<{
 
   updateComponentFilters: (componentFilters: Array<ComponentFilter>) => void =
     componentFilters => {
-      for (const rendererID in this._rendererInterfaces) {
+      for (const rendererIDString in this._rendererInterfaces) {
+        const rendererID = +rendererIDString;
         const renderer = ((this._rendererInterfaces[
           (rendererID: any)
         ]: any): RendererInterface);
+        if (this._lastSelectedRendererID === rendererID) {
+          // Changing component filters will unmount and remount the DevTools tree.
+          // Track the last selection's path so we can restore the selection.
+          const path = renderer.getPathForElement(this._lastSelectedElementID);
+          if (path !== null) {
+            renderer.setTrackedPath(path);
+            this._persistedSelection = {
+              rendererID,
+              path,
+            };
+          }
+        }
         renderer.updateComponentFilters(componentFilters);
       }
     };