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( + <React.Fragment> + <Child key="A" /> + <ErrorOnce key="B" /> + <Child key="C" /> + <ErrorOnce key="D" /> + </React.Fragment>, + container, + ), + ), + ); + + let renderer; + utils.act(() => (renderer = TestRenderer.create(<Contexts />))); + expect(state).toMatchInlineSnapshot(` + ✕ 2, ⚠ 0 + [root] + <Child key="A"> + <ErrorOnce key="B"> ✕ + <Child key="C"> + <ErrorOnce key="D"> ✕ + `); + + // Select a child + selectNextErrorOrWarning(); + utils.act(() => renderer.update(<Contexts />)); + expect(state).toMatchInlineSnapshot(` + ✕ 2, ⚠ 0 + [root] + <Child key="A"> + → <ErrorOnce key="B"> ✕ + <Child key="C"> + <ErrorOnce key="D"> ✕ + `); + + // Re-order the tree and ensure indices are updated. + withErrorsOrWarningsIgnored(['test-only:'], () => + utils.act(() => + legacyRender( + <React.Fragment> + <ErrorOnce key="B" /> + <Child key="A" /> + <ErrorOnce key="D" /> + <Child key="C" /> + </React.Fragment>, + container, + ), + ), + ); + expect(state).toMatchInlineSnapshot(` + ✕ 2, ⚠ 0 + [root] + → <ErrorOnce key="B"> ✕ + <Child key="A"> + <ErrorOnce key="D"> ✕ + <Child key="C"> + `); + + // Select the next child and ensure the index doesn't break. + selectNextErrorOrWarning(); + expect(state).toMatchInlineSnapshot(` + ✕ 2, ⚠ 0 + [root] + <ErrorOnce key="B"> ✕ + <Child key="A"> + → <ErrorOnce key="D"> ✕ + <Child key="C"> + `); + + // Re-order the tree and ensure indices are updated. + withErrorsOrWarningsIgnored(['test-only:'], () => + utils.act(() => + legacyRender( + <React.Fragment> + <ErrorOnce key="D" /> + <ErrorOnce key="B" /> + <Child key="A" /> + <Child key="C" /> + </React.Fragment>, + container, + ), + ), + ); + expect(state).toMatchInlineSnapshot(` + ✕ 2, ⚠ 0 + [root] + → <ErrorOnce key="D"> ✕ + <ErrorOnce key="B"> ✕ + <Child key="A"> + <Child key="C"> + `); + }); + 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]); };