From 6349d90656190181dfb3cf1c4391ea38b026734c Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Fri, 20 Aug 2021 09:38:23 -0700 Subject: [PATCH] Reset cached error/warning element indices in Store Any time the tree changes (elements added, removed, or re-ordered) our cached id-to-index map might be invalid. Reset it more aggressively on tree operations and recompute it lazily. --- .../src/__tests__/treeContext-test.js | 105 ++++++++++++++++++ .../src/devtools/store.js | 70 ++++++------ 2 files changed, 139 insertions(+), 36 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/treeContext-test.js b/packages/react-devtools-shared/src/__tests__/treeContext-test.js index 30d21c0335ddb..36f14d8c7d66d 100644 --- a/packages/react-devtools-shared/src/__tests__/treeContext-test.js +++ b/packages/react-devtools-shared/src/__tests__/treeContext-test.js @@ -2177,6 +2177,111 @@ describe('TreeListContext', () => { `); }); + it('should update correctly when elements are re-ordered', () => { + const container = document.createElement('div'); + function ErrorOnce() { + const didErroRef = React.useRef(false); + if (!didErroRef.current) { + didErroRef.current = true; + console.error('test-only:one-time-error'); + } + return null; + } + withErrorsOrWarningsIgnored(['test-only:'], () => + utils.act(() => + legacyRender( + + + + + + , + container, + ), + ), + ); + + let renderer; + utils.act(() => (renderer = TestRenderer.create())); + expect(state).toMatchInlineSnapshot(` + ✕ 2, ⚠ 0 + [root] + + ✕ + + ✕ + `); + + // Select a child + selectNextErrorOrWarning(); + utils.act(() => renderer.update()); + expect(state).toMatchInlineSnapshot(` + ✕ 2, ⚠ 0 + [root] + + → ✕ + + ✕ + `); + + // Re-order the tree and ensure indices are updated. + withErrorsOrWarningsIgnored(['test-only:'], () => + utils.act(() => + legacyRender( + + + + + + , + container, + ), + ), + ); + expect(state).toMatchInlineSnapshot(` + ✕ 2, ⚠ 0 + [root] + → ✕ + + ✕ + + `); + + // Select the next child and ensure the index doesn't break. + selectNextErrorOrWarning(); + expect(state).toMatchInlineSnapshot(` + ✕ 2, ⚠ 0 + [root] + ✕ + + → ✕ + + `); + + // Re-order the tree and ensure indices are updated. + withErrorsOrWarningsIgnored(['test-only:'], () => + utils.act(() => + legacyRender( + + + + + + , + container, + ), + ), + ); + expect(state).toMatchInlineSnapshot(` + ✕ 2, ⚠ 0 + [root] + → ✕ + ✕ + + + `); + }); + it('should update select and auto-expand parts components within hidden parts of the tree', () => { const Wrapper = ({children}) => children; diff --git a/packages/react-devtools-shared/src/devtools/store.js b/packages/react-devtools-shared/src/devtools/store.js index 7cd5c41e8b0d7..217abe7f72777 100644 --- a/packages/react-devtools-shared/src/devtools/store.js +++ b/packages/react-devtools-shared/src/devtools/store.js @@ -57,6 +57,8 @@ const LOCAL_STORAGE_COLLAPSE_ROOTS_BY_DEFAULT_KEY = const LOCAL_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY = 'React::DevTools::recordChangeDescriptions'; +type ErrorAndWarningTuples = Array<{|id: number, index: number|}>; + type Config = {| checkBridgeProtocolCompatibility?: boolean, isProfiling?: boolean, @@ -94,7 +96,7 @@ export default class Store extends EventEmitter<{| // Computed whenever _errorsAndWarnings Map changes. _cachedErrorCount: number = 0; _cachedWarningCount: number = 0; - _cachedErrorAndWarningTuples: Array<{|id: number, index: number|}> = []; + _cachedErrorAndWarningTuples: ErrorAndWarningTuples | null = null; // Should new nodes be collapsed by default when added to the tree? _collapseNodesByDefault: boolean = true; @@ -510,7 +512,34 @@ export default class Store extends EventEmitter<{| // Returns a tuple of [id, index] getElementsWithErrorsAndWarnings(): Array<{|id: number, index: number|}> { - return this._cachedErrorAndWarningTuples; + if (this._cachedErrorAndWarningTuples !== null) { + return this._cachedErrorAndWarningTuples; + } else { + const errorAndWarningTuples: ErrorAndWarningTuples = []; + + this._errorsAndWarnings.forEach((_, id) => { + const index = this.getIndexOfElementID(id); + if (index !== null) { + let low = 0; + let high = errorAndWarningTuples.length; + while (low < high) { + const mid = (low + high) >> 1; + if (errorAndWarningTuples[mid].index > index) { + high = mid; + } else { + low = mid + 1; + } + } + + errorAndWarningTuples.splice(low, 0, {id, index}); + } + }); + + // Cache for later (at least until the tree changes again). + this._cachedErrorAndWarningTuples = errorAndWarningTuples; + + return errorAndWarningTuples; + } } getErrorAndWarningCountForElementID( @@ -1124,6 +1153,9 @@ export default class Store extends EventEmitter<{| this._revision++; + // Any time the tree changes (e.g. elements added, removed, or reordered) cached inidices may be invalid. + this._cachedErrorAndWarningTuples = null; + if (haveErrorsOrWarningsChanged) { let errorCount = 0; let warningCount = 0; @@ -1135,28 +1167,6 @@ export default class Store extends EventEmitter<{| this._cachedErrorCount = errorCount; this._cachedWarningCount = warningCount; - - const errorAndWarningTuples: Array<{|id: number, index: number|}> = []; - - this._errorsAndWarnings.forEach((_, id) => { - const index = this.getIndexOfElementID(id); - if (index !== null) { - let low = 0; - let high = errorAndWarningTuples.length; - while (low < high) { - const mid = (low + high) >> 1; - if (errorAndWarningTuples[mid].index > index) { - high = mid; - } else { - low = mid + 1; - } - } - - errorAndWarningTuples.splice(low, 0, {id, index}); - } - }); - - this._cachedErrorAndWarningTuples = errorAndWarningTuples; } if (haveRootsChanged) { @@ -1187,18 +1197,6 @@ export default class Store extends EventEmitter<{| console.groupEnd(); } - const indicesOfCachedErrorsOrWarningsAreStale = - !haveErrorsOrWarningsChanged && - (addedElementIDs.length > 0 || removedElementIDs.size > 0); - if (indicesOfCachedErrorsOrWarningsAreStale) { - this._cachedErrorAndWarningTuples.forEach(entry => { - const index = this.getIndexOfElementID(entry.id); - if (index !== null) { - entry.index = index; - } - }); - } - this.emit('mutated', [addedElementIDs, removedElementIDs]); };