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: update error indices when elements are added/removed #22144

Merged
merged 3 commits into from
Aug 20, 2021
Merged

DevTools: update error indices when elements are added/removed #22144

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e7360ff
Current behavior for index change
eps1lon Aug 20, 2021
8995c81
fix: update error indices when elements are added/removed
eps1lon Aug 20, 2021
42d4bd6
get flow in line
eps1lon Aug 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions packages/react-devtools-shared/src/__tests__/treeContext-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2119,6 +2119,64 @@ describe('TreeListContext', () => {
`);
});

it('should update correctly when elements are added/removed', () => {
const container = document.createElement('div');
let errored = false;
function ErrorOnce() {
if (!errored) {
errored = true;
console.error('test-only:one-time-error');
}
return null;
}
withErrorsOrWarningsIgnored(['test-only:'], () =>
utils.act(() =>
legacyRender(
<React.Fragment>
<ErrorOnce key="error" />
</React.Fragment>,
container,
),
),
);

let renderer;
utils.act(() => (renderer = TestRenderer.create(<Contexts />)));
expect(state).toMatchInlineSnapshot(`
✕ 1, ⚠ 0
[root]
<ErrorOnce key="error"> ✕
`);

withErrorsOrWarningsIgnored(['test-only:'], () =>
utils.act(() =>
legacyRender(
<React.Fragment>
<Child />
<ErrorOnce key="error" />
</React.Fragment>,
container,
),
),
);

utils.act(() => renderer.update(<Contexts />));
expect(state).toMatchInlineSnapshot(`
✕ 1, ⚠ 0
[root]
<Child>
<ErrorOnce key="error"> ✕
`);

selectNextErrorOrWarning();
expect(state).toMatchInlineSnapshot(`
✕ 1, ⚠ 0
[root]
<Child>
→ <ErrorOnce key="error"> ✕
`);
});

it('should update select and auto-expand parts components within hidden parts of the tree', () => {
const Wrapper = ({children}) => children;

Expand Down
12 changes: 12 additions & 0 deletions packages/react-devtools-shared/src/devtools/store.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1187,6 +1187,18 @@ export default class Store extends EventEmitter<{|
console.groupEnd();
}

const indicesOfCachedErrorsOrWarningsAreStale =
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is also missing the case where we just re-order elements. We could let the operations decide when indicesOfCachedErrorsOrWarningsAreStale should be true instead but this feels a bit too brittle (though we have the same problem with haveErrorsOrWarningsChanged).

The other solution would be to rethink the index caching strategy alltogether. So just cache the IDs and get the indices (cached) when necessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good point.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH we should probably just store the ID and look up the index on the fly each time (no caching).

Copy link
Collaborator Author

@eps1lon eps1lon Aug 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH we should probably just store the ID and look up the index on the fly each time (no caching).

That was my thinking as well. Though it would need some additional work since we currently assume that errorsOrWarnings are sorted by indices.

So maybe we keep an unsorted set with all the fiber ids with errors or warnings and a separate list that is sorted index. When we handle operations we just update the set and once the user wants to loop through them we create a (cached) list that has them ordered by index.

!haveErrorsOrWarningsChanged &&
(addedElementIDs.length > 0 || removedElementIDs.size > 0);
if (indicesOfCachedErrorsOrWarningsAreStale) {
this._cachedErrorAndWarningTuples.forEach(entry => {
const index = this.getIndexOfElementID(entry.id);
if (index !== null) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to guard here or would this be bug at this point and we should include a warning?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should ever be able to happen, and we'd just have this check to satisfy Flow.

entry.index = index;
}
});
}

this.emit('mutated', [addedElementIDs, removedElementIDs]);
};

Expand Down