Refactored next/prev index calculation and memoized error/warning count

eps1lon · Dec 18, 2020 · a27fc21 · a27fc21
1 parent 4c9bf65
commit a27fc21
Show file tree

Hide file tree

Showing 8 changed files with 256 additions and 142 deletions.
diff --git a/packages/react-devtools-shared/src/__tests__/store-test.js b/packages/react-devtools-shared/src/__tests__/store-test.js
@@ -1304,7 +1304,8 @@ describe('Store', () => {
         act(() => ReactDOM.render(<React.Fragment />, container));
       });
       expect(store).toMatchInlineSnapshot(`[root]`);
-      expect(store.errorsAndWarnings).toMatchInlineSnapshot(`Map {}`);
+      expect(store.errorCount).toBe(0);
+      expect(store.warningCount).toBe(0);
     });
   });
 });
diff --git a/packages/react-devtools-shared/src/__tests__/storeComponentFilters-test.js b/packages/react-devtools-shared/src/__tests__/storeComponentFilters-test.js
@@ -398,7 +398,8 @@ describe('Store component filters', () => {
       });
 
       expect(store).toMatchInlineSnapshot(`[root]`);
-      expect(store.errorsAndWarnings).toMatchInlineSnapshot(`Map {}`);
+      expect(store.errorCount).toBe(0);
+      expect(store.warningCount).toBe(0);
 
       act(() => (store.componentFilters = []));
       expect(store).toMatchInlineSnapshot(`
@@ -434,7 +435,8 @@ describe('Store component filters', () => {
           ]),
       );
       expect(store).toMatchInlineSnapshot(`[root]`);
-      expect(store.errorsAndWarnings).toMatchInlineSnapshot(`Map {}`);
+      expect(store.errorCount).toBe(0);
+      expect(store.warningCount).toBe(0);
 
       act(() => (store.componentFilters = []));
       expect(store).toMatchInlineSnapshot(`

diff --git a/packages/react-devtools-shared/src/__tests__/treeContext-test.js b/packages/react-devtools-shared/src/__tests__/treeContext-test.js
@@ -1659,6 +1659,64 @@ describe('TreeListContext', () => {
       `);
     });
 
+    it('should select the next or previous element relative to the current selection', () => {
+      withErrorsOrWarningsIgnored(['test-only:'], () =>
+        utils.act(() =>
+          ReactDOM.render(
+            <React.Fragment>
+              <Child />
+              <Child logWarning={true} />
+              <Child />
+              <Child logError={true} />
+              <Child />
+            </React.Fragment>,
+            document.createElement('div'),
+          ),
+        ),
+      );
+
+      utils.act(() => TestRenderer.create(<Contexts />));
+      utils.act(() => dispatch({type: 'SELECT_ELEMENT_AT_INDEX', payload: 2}));
+      expect(state).toMatchInlineSnapshot(`
+        [root] ✕ 1, ⚠ 1
+             <Child>
+             <Child> ⚠
+        →    <Child>
+             <Child> ✕
+             <Child>
+      `);
+
+      selectNextErrorOrWarning();
+      expect(state).toMatchInlineSnapshot(`
+        [root] ✕ 1, ⚠ 1
+             <Child>
+             <Child> ⚠
+             <Child>
+        →    <Child> ✕
+             <Child>
+      `);
+
+      utils.act(() => dispatch({type: 'SELECT_ELEMENT_AT_INDEX', payload: 2}));
+      expect(state).toMatchInlineSnapshot(`
+        [root] ✕ 1, ⚠ 1
+             <Child>
+             <Child> ⚠
+        →    <Child>
+             <Child> ✕
+             <Child>
+      `);
+
+      selectPreviousErrorOrWarning();
+      expect(state).toMatchInlineSnapshot(`
+        [root] ✕ 1, ⚠ 1
+             <Child>
+        →    <Child> ⚠
+             <Child>
+             <Child> ✕
+             <Child>
+      `);
+    });
+
     it('should update correctly when errors/warnings are cleared for a fiber in the list', () => {
       withErrorsOrWarningsIgnored(['test-only:'], () =>
         utils.act(() =>
@@ -2088,16 +2146,16 @@ describe('TreeListContext', () => {
         jest.runAllTimers();
 
         expect(state).toMatchInlineSnapshot(`
-        [root]
-             <Suspense>
-      `);
+                  [root]
+                       <Suspense>
+              `);
 
         selectNextErrorOrWarning();
 
         expect(state).toMatchInlineSnapshot(`
-        [root]
-             <Suspense>
-      `);
+                  [root]
+                       <Suspense>
+              `);
       });
 
       it('should properly handle errors/warnings from components that dont mount because of Suspense', async () => {
@@ -2122,9 +2180,9 @@ describe('TreeListContext', () => {
         utils.act(() => TestRenderer.create(<Contexts />));
 
         expect(state).toMatchInlineSnapshot(`
-        [root]
-             <Suspense>
-      `);
+                  [root]
+                       <Suspense>
+              `);
 
         await Promise.resolve();
         withErrorsOrWarningsIgnored(['test-only:'], () =>
@@ -2140,11 +2198,11 @@ describe('TreeListContext', () => {
         );
 
         expect(state).toMatchInlineSnapshot(`
-        [root] ✕ 0, ⚠ 2
-           ▾ <Suspense>
-               <Child> ⚠
-               <Child>
-      `);
+                  [root] ✕ 0, ⚠ 2
+                     ▾ <Suspense>
+                         <Child> ⚠
+                         <Child>
+              `);
       });
 
       it('should properly show errors/warnings from components in the Suspense fallback tree', async () => {
@@ -2170,11 +2228,11 @@ describe('TreeListContext', () => {
         utils.act(() => TestRenderer.create(<Contexts />));
 
         expect(state).toMatchInlineSnapshot(`
-        [root] ✕ 1, ⚠ 0
-           ▾ <Suspense>
-             ▾ <Fallback>
-                 <Child> ✕
-      `);
+                  [root] ✕ 1, ⚠ 0
+                     ▾ <Suspense>
+                       ▾ <Fallback>
+                           <Child> ✕
+              `);
 
         await Promise.resolve();
         withErrorsOrWarningsIgnored(['test-only:'], () =>
@@ -2189,10 +2247,10 @@ describe('TreeListContext', () => {
         );
 
         expect(state).toMatchInlineSnapshot(`
-        [root]
-           ▾ <Suspense>
-               <Child>
-      `);
+                  [root]
+                     ▾ <Suspense>
+                         <Child>
+              `);
       });
     });
 
@@ -2236,15 +2294,15 @@ describe('TreeListContext', () => {
         utils.act(() => TestRenderer.create(<Contexts />));
 
         expect(store).toMatchInlineSnapshot(`
-        [root] ✕ 1, ⚠ 0
-            <ErrorBoundary> ✕
-      `);
+                  [root] ✕ 1, ⚠ 0
+                      <ErrorBoundary> ✕
+              `);
 
         selectNextErrorOrWarning();
         expect(state).toMatchInlineSnapshot(`
-        [root] ✕ 1, ⚠ 0
-        →    <ErrorBoundary> ✕
-      `);
+                  [root] ✕ 1, ⚠ 0
+                  →    <ErrorBoundary> ✕
+              `);
 
         utils.act(() => ReactDOM.unmountComponentAtNode(container));
         expect(state).toMatchInlineSnapshot(``);
@@ -2298,15 +2356,15 @@ describe('TreeListContext', () => {
         utils.act(() => TestRenderer.create(<Contexts />));
 
         expect(store).toMatchInlineSnapshot(`
-        [root] ✕ 1, ⚠ 0
-            <ErrorBoundary> ✕
-      `);
+                  [root] ✕ 1, ⚠ 0
+                      <ErrorBoundary> ✕
+              `);
 
         selectNextErrorOrWarning();
         expect(state).toMatchInlineSnapshot(`
-        [root] ✕ 1, ⚠ 0
-        →    <ErrorBoundary> ✕
-      `);
+                  [root] ✕ 1, ⚠ 0
+                  →    <ErrorBoundary> ✕
+              `);
 
         utils.act(() => ReactDOM.unmountComponentAtNode(container));
         expect(state).toMatchInlineSnapshot(``);
@@ -2355,17 +2413,17 @@ describe('TreeListContext', () => {
         utils.act(() => TestRenderer.create(<Contexts />));
 
         expect(store).toMatchInlineSnapshot(`
-        [root] ✕ 2, ⚠ 0
-          ▾ <ErrorBoundary> ✕
-              <Child> ✕
-      `);
+                  [root] ✕ 2, ⚠ 0
+                    ▾ <ErrorBoundary> ✕
+                        <Child> ✕
+              `);
 
         selectNextErrorOrWarning();
         expect(state).toMatchInlineSnapshot(`
-        [root] ✕ 2, ⚠ 0
-        →  ▾ <ErrorBoundary> ✕
-               <Child> ✕
-      `);
+                  [root] ✕ 2, ⚠ 0
+                  →  ▾ <ErrorBoundary> ✕
+                         <Child> ✕
+              `);
       });
     });
   });

diff --git a/packages/react-devtools-shared/src/devtools/store.js b/packages/react-devtools-shared/src/devtools/store.js
@@ -80,6 +80,10 @@ export default class Store extends EventEmitter<{|
 |}> {
   _bridge: FrontendBridge;
 
+  // Computed whenever _errorsAndWarnings Map changes.
+  _cachedErrorCount: number = 0;
+  _cachedWarningCount: number = 0;
+
   // Should new nodes be collapsed by default when added to the tree?
   _collapseNodesByDefault: boolean = true;
 
@@ -297,8 +301,8 @@ export default class Store extends EventEmitter<{|
     this.emit('componentFilters');
   }
 
-  get errorsAndWarnings(): Map<number, {errors: number, warnings: number}> {
-    return this._errorsAndWarnings;
+  get errorCount(): number {
+    return this._cachedErrorCount;
   }
 
   get hasOwnerMetadata(): boolean {
@@ -369,6 +373,10 @@ export default class Store extends EventEmitter<{|
     return this._unsupportedRendererVersionDetected;
   }
 
+  get warningCount(): number {
+    return this._cachedWarningCount;
+  }
+
   clearErrorsAndWarnings(): void {
     this._rootIDToRendererID.forEach(rendererID => {
       this._bridge.send('clearErrorsAndWarnings', {
@@ -473,6 +481,45 @@ export default class Store extends EventEmitter<{|
     return element;
   }
 
+  // Returns a tuple of [id, index]
+  getElementsWithErrorsAndWarnings(): Array<{|id: number, index: number|}> {
+    const sortedArray: Array<{|id: number, index: number|}> = [];
+
+    this._errorsAndWarnings.forEach((_, id) => {
+      const index = this.getIndexOfElementID(id);
+      if (index !== null) {
+        let low = 0;
+        let high = sortedArray.length;
+        while (low < high) {
+          const mid = (low + high) >> 1;
+          if (sortedArray[mid].index > index) {
+            high = mid;
+          } else {
+            low = mid + 1;
+          }
+        }
+
+        sortedArray.splice(low, 0, {id, index});
+      }
+    });
+
+    return sortedArray;
+  }
+
+  getErrorAndWarningCountForElementID(
+    id: number,
+  ): {|errorCount: number, warningCount: number|} {
+    const map = this._errorsAndWarnings.get(id);
+    if (map != null) {
+      return {
+        errorCount: map.errors,
+        warningCount: map.warnings,
+      };
+    } else {
+      return {errorCount: 0, warningCount: 0};
+    }
+  }
+
   getIndexOfElementID(id: number): number | null {
     const element = this.getElementByID(id);
 
@@ -757,6 +804,7 @@ export default class Store extends EventEmitter<{|
     }
 
     let haveRootsChanged = false;
+    let haveErrorsOrWarningsChanged = false;
 
     // The first two values are always rendererID and rootID
     const rendererID = operations[0];
@@ -961,6 +1009,7 @@ export default class Store extends EventEmitter<{|
 
             if (this._errorsAndWarnings.has(id)) {
               this._errorsAndWarnings.delete(id);
+              haveErrorsOrWarningsChanged = true;
             }
           }
           break;
@@ -1053,6 +1102,7 @@ export default class Store extends EventEmitter<{|
           } else if (this._errorsAndWarnings.has(id)) {
             this._errorsAndWarnings.delete(id);
           }
+          haveErrorsOrWarningsChanged = true;
           break;
         default:
           throw Error(`Unsupported Bridge operation ${operation}`);
@@ -1061,6 +1111,19 @@ export default class Store extends EventEmitter<{|
 
     this._revision++;
 
+    if (haveErrorsOrWarningsChanged) {
+      let errorCount = 0;
+      let warningCount = 0;
+
+      this._errorsAndWarnings.forEach(({errors, warnings}) => {
+        errorCount += errors;
+        warningCount += warnings;
+      });
+
+      this._cachedErrorCount = errorCount;
+      this._cachedWarningCount = warningCount;
+    }
+
     if (haveRootsChanged) {
       const prevSupportsProfiling = this._supportsProfiling;