Skip to content

Commit

Permalink
[DevTools] Refactor Error / Warning Count Tracking (#30899)
Browse files Browse the repository at this point in the history
We can simplify this tracking by not having a separate pending set of
logs and the logs tracked per instance and instead we just track the
logs per Fiber. This avoids the need to move it back into the pending
set after unmounts in case a Fiber is reparented.

The main motivation for this is to unify with an upcoming tracking of
logs for Server Components. For those it doesn't make sense to move them
into a per instance set and because the same Server Component - and its
logs - may appear more than once. So no particular instance should steal
it.

The second part of this change is that instead of looking up the
instance from fiber, which requires the fiberToFiberInstanceMap, we
instead look up if a component has any new logs when we traverse it in
the commit phase. After all for a component to have had a log it must
have updated. This is a similar technique to #30897. This technique also
works for Server Components without having to maintain a one to many
relationship from ComponentInfo to VirtualInstance. So it unifies them.

Normally this look up would be fast since the `fiberToComponentsLogs`
set would be empty and so doesn't add any significant weight to the
commit phase. If there's a ton of logs on many different components then
it's not great since it would slow down the commit phase but that's not
what we expect to see so in typical usage, this is better.

There is an unfortunate consequence though which is that
`console.warn/error` in passive effects (i.e. `useEffect`) wouldn't be
picked up because currently we traverse the logs in
`handleCommitFiberRoot` which is too early. If we moved that to
`handlePostCommitFiberRoot` this wouldn't be a problem. In the meantime,
I just detect this and do a brute force flush by walking all mounted
instances if there's a `console.warn/error` inside a passive effect.

If we ever add "owners" to event handlers that are triggered outside the
render/commit phases (like `<div onClick={...}>`) and we want to
associate error/warnings in those, we'd need a different technique to
ensure those get flushed in time.
  • Loading branch information
sebmarkbage authored Sep 9, 2024
1 parent e210d08 commit e07235b
Show file tree
Hide file tree
Showing 2 changed files with 183 additions and 320 deletions.
28 changes: 4 additions & 24 deletions packages/react-devtools-shared/src/__tests__/store-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1817,13 +1817,8 @@ describe('Store', () => {
jest.runOnlyPendingTimers();
}

// Gross abstraction around pending passive warning/error delay.
function flushPendingPassiveErrorAndWarningCounts() {
jest.advanceTimersByTime(1000);
}

// @reactVersion >= 18.0
it('are counted (after a delay)', () => {
it('are counted (after no delay)', () => {
function Example() {
React.useEffect(() => {
console.error('test-only: passive error');
Expand All @@ -1838,13 +1833,6 @@ describe('Store', () => {
}, false);
});
flushPendingBridgeOperations();
expect(store).toMatchInlineSnapshot(`
[root]
<Example>
`);

// After a delay, passive effects should be committed as well
act(flushPendingPassiveErrorAndWarningCounts, false);
expect(store).toMatchInlineSnapshot(`
✕ 1, ⚠ 1
[root]
Expand Down Expand Up @@ -1879,8 +1867,9 @@ describe('Store', () => {
}, false);
flushPendingBridgeOperations();
expect(store).toMatchInlineSnapshot(`
✕ 1, ⚠ 1
[root]
<Example>
<Example> ✕⚠
`);

// Before warnings and errors have flushed, flush another commit.
Expand All @@ -1894,22 +1883,13 @@ describe('Store', () => {
}, false);
flushPendingBridgeOperations();
expect(store).toMatchInlineSnapshot(`
1, ⚠ 1
2, ⚠ 2
[root]
<Example> ✕⚠
<Noop>
`);
});

// After a delay, passive effects should be committed as well
act(flushPendingPassiveErrorAndWarningCounts, false);
expect(store).toMatchInlineSnapshot(`
✕ 2, ⚠ 2
[root]
<Example> ✕⚠
<Noop>
`);

act(() => unmount());
expect(store).toMatchInlineSnapshot(``);
});
Expand Down
Loading

0 comments on commit e07235b

Please sign in to comment.