Skip to content

Commit

Permalink
Cleaned up renderer code and added additional unit test
Browse files Browse the repository at this point in the history
  • Loading branch information
Brian Vaughn committed Mar 5, 2021
1 parent 8716606 commit cc7ba6a
Show file tree
Hide file tree
Showing 2 changed files with 135 additions and 75 deletions.
131 changes: 99 additions & 32 deletions packages/react-devtools-shared/src/__tests__/store-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1075,43 +1075,110 @@ describe('Store', () => {
`);
});

it('during passive get counted (after a delay)', () => {
function Example() {
React.useEffect(() => {
console.error('test-only: passive error');
console.warn('test-only: passive warning');
});
return null;
describe('during passive effects', () => {
function flushPendingBridgeOperations() {
jest.runOnlyPendingTimers();
}
const container = document.createElement('div');

withErrorsOrWarningsIgnored(['test-only:'], () => {
act(() => {
ReactDOM.render(<Example />, container);
// flush bridge operations
jest.runOnlyPendingTimers();
}, false);
});

expect(store).toMatchInlineSnapshot(`
[root]
<Example>
`);

// flush count after delay
act(() => {
// Gross abstraction around pending passive warning/error delay.
function flushPendingPassiveErrorAndWarningCounts() {
jest.advanceTimersByTime(1000);
}, false);
}

// After a delay, passive effects should be committed as well
expect(store).toMatchInlineSnapshot(`
✕ 1, ⚠ 1
[root]
<Example> ✕⚠
`);
it('are counted (after a delay)', () => {
function Example() {
React.useEffect(() => {
console.error('test-only: passive error');
console.warn('test-only: passive warning');
});
return null;
}
const container = document.createElement('div');

withErrorsOrWarningsIgnored(['test-only:'], () => {
act(() => {
ReactDOM.render(<Example />, container);
}, 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]
<Example> ✕⚠
`);

act(() => ReactDOM.unmountComponentAtNode(container));
expect(store).toMatchInlineSnapshot(``);
});

act(() => ReactDOM.unmountComponentAtNode(container));
expect(store).toMatchInlineSnapshot(``);
it('are flushed early when there is a new commit', () => {
function Example() {
React.useEffect(() => {
console.error('test-only: passive error');
console.warn('test-only: passive warning');
});
return null;
}

function Noop() {
return null;
}

const container = document.createElement('div');

withErrorsOrWarningsIgnored(['test-only:'], () => {
act(() => {
ReactDOM.render(
<>
<Example />
</>,
container,
);
}, false);
flushPendingBridgeOperations();
expect(store).toMatchInlineSnapshot(`
[root]
<Example>
`);

// Before warnings and errors have flushed, flush another commit.
act(() => {
ReactDOM.render(
<>
<Example />
<Noop />
</>,
container,
);
}, false);
flushPendingBridgeOperations();
expect(store).toMatchInlineSnapshot(`
✕ 1, ⚠ 1
[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(() => ReactDOM.unmountComponentAtNode(container));
expect(store).toMatchInlineSnapshot(``);
});
});

it('from react get counted', () => {
Expand Down
79 changes: 36 additions & 43 deletions packages/react-devtools-shared/src/backend/renderer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -631,7 +631,8 @@ export function attach(
// In this case, we should wait until the rest of the passive effects have run,
// but we shouldn't wait until the next commit because that might be a long time.
// This would also cause "tearing" between an inspected Component and the tree view.
// Then again we don't want to flush too soon because rendering might stil be going on.
// Then again we don't want to flush too soon because this could be an error during async rendering.
// Use a debounce technique to ensure that we'll eventually flush.
flushPendingErrorsAndWarningsAfterDelay();
}

Expand Down Expand Up @@ -1205,10 +1206,12 @@ export function attach(
}
}

const pendingOperations: Array<number> = [];
type OperationsArray = Array<number>;

const pendingOperations: OperationsArray = [];
const pendingRealUnmountedIDs: Array<number> = [];
const pendingSimulatedUnmountedIDs: Array<number> = [];
let pendingOperationsQueue: Array<Array<number>> | null = [];
let pendingOperationsQueue: Array<OperationsArray> | null = [];
const pendingStringTable: Map<string, number> = new Map();
let pendingStringTableLength: number = 0;
let pendingUnmountedRootID: number | null = null;
Expand All @@ -1225,55 +1228,57 @@ export function attach(
pendingOperations.push(op);
}

function flushOrQueueOperations(operations: OperationsArray): void {
if (pendingOperationsQueue !== null) {
pendingOperationsQueue.push(operations);
} else {
hook.emit('operations', operations);
}
}

let flushPendingErrorsAndWarningsAfterDelayTimeoutID = null;
function flushPendingErrorsAndWarningsAfterDelay() {

function clearPendingErrorsAndWarningsAfterDelay() {
if (flushPendingErrorsAndWarningsAfterDelayTimeoutID !== null) {
clearTimeout(flushPendingErrorsAndWarningsAfterDelayTimeoutID);
}
}

function flushPendingErrorsAndWarningsAfterDelay() {
clearPendingErrorsAndWarningsAfterDelay();

flushPendingErrorsAndWarningsAfterDelayTimeoutID = setTimeout(() => {
flushPendingErrorsAndWarningsAfterDelayTimeoutID = null;

if (pendingOperations.length > 0) {
// On the off chance that somethign else has pushed pending operations,
// On the off chance that something else has pushed pending operations,
// we should bail on warnings; it's probably not safe to push midway.
return;
}

recordPendingErrorsAndWarnings();

if (pendingOperations.length === 0) {
// No warnings or errors to flush.
// No warnings or errors to flush; we can bail out early here too.
return;
}

const operations = new Array(2 + 1 + pendingOperations.length);

// Identify which renderer this update is coming from.
// This enables roots to be mapped to renderers,
// Which in turn enables fiber props, states, and hooks to be inspected.
let i = 0;
operations[i++] = rendererID;
operations[i++] = currentRootID; // Use this ID in case the root was unmounted!
operations[i++] = 0; // String table size
// We can create a smaller operations array than flushPendingEvents()
// because we only need to flush warning and error counts.
// Only a few pieces of fixed information are required up front.
const operations: OperationsArray = new Array(
3 + pendingOperations.length,
);
operations[0] = rendererID;
operations[1] = currentRootID; // Use this ID in case the root was unmounted!
operations[2] = 0; // String table size

// Fill in the rest of the operations.
for (let j = 0; j < pendingOperations.length; j++) {
operations[i + j] = pendingOperations[j];
operations[3 + j] = pendingOperations[j];
}

// Let the frontend know about tree operations.
// The first value in this array will identify which root it corresponds to,
// so we do no longer need to dispatch a separate root-committed event.
if (pendingOperationsQueue !== null) {
// Until the frontend has been connected, store the tree operations.
// This will let us avoid walking the tree later when the frontend connects,
// and it enables the Profiler's reload-and-profile functionality to work as well.
pendingOperationsQueue.push(operations);
} else {
// If we've already connected to the frontend, just pass the operations through.
hook.emit('operations', operations);
}
flushOrQueueOperations(operations);

pendingOperations.length = 0;
}, 1000);
Expand All @@ -1291,10 +1296,7 @@ export function attach(
}

function recordPendingErrorsAndWarnings() {
if (flushPendingErrorsAndWarningsAfterDelayTimeoutID !== null) {
clearTimeout(flushPendingErrorsAndWarningsAfterDelayTimeoutID);
flushPendingErrorsAndWarningsAfterDelayTimeoutID = null;
}
clearPendingErrorsAndWarningsAfterDelay();

fibersWithChangedErrorOrWarningCounts.forEach(fiberID => {
const fiber = idToFiberMap.get(fiberID);
Expand Down Expand Up @@ -1432,18 +1434,9 @@ export function attach(
i += pendingOperations.length;

// Let the frontend know about tree operations.
// The first value in this array will identify which root it corresponds to,
// so we do no longer need to dispatch a separate root-committed event.
if (pendingOperationsQueue !== null) {
// Until the frontend has been connected, store the tree operations.
// This will let us avoid walking the tree later when the frontend connects,
// and it enables the Profiler's reload-and-profile functionality to work as well.
pendingOperationsQueue.push(operations);
} else {
// If we've already connected to the frontend, just pass the operations through.
hook.emit('operations', operations);
}
flushOrQueueOperations(operations);

// Reset all of the pending state now that we've told the frontend about it.
pendingOperations.length = 0;
pendingRealUnmountedIDs.length = 0;
pendingSimulatedUnmountedIDs.length = 0;
Expand Down

0 comments on commit cc7ba6a

Please sign in to comment.