Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DevTools: Reset cached indices in Store after elements reordered #22147

Merged
merged 1 commit into from Aug 20, 2021
Merged

DevTools: Reset cached indices in Store after elements reordered #22147

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
105 changes: 105 additions & 0 deletions packages/react-devtools-shared/src/__tests__/treeContext-test.js
View file
Open in desktop
Expand Up @@ -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;

Expand Down
70 changes: 34 additions & 36 deletions packages/react-devtools-shared/src/devtools/store.js
View file
Open in desktop
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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;
Expand All @@ -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) {
Expand Down Expand Up @@ -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]);
};

Expand Down