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

Partial fix DevTools Profiler "Could not find node…" error #17759

Merged
merged 1 commit into from
Jan 3, 2020
Merged

Partial fix DevTools Profiler "Could not find node…" error #17759

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/react-devtools-shared/src/__tests__/profilerStore-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,5 +118,6 @@ describe('ProfilerStore', () => {
const root = store.roots[0];
const data = store.profilerStore.getDataForRoot(root);
expect(data.commitData).toHaveLength(1);
expect(data.operations).toHaveLength(1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check fails without the newly-added bailout fix.

});
});
27 changes: 24 additions & 3 deletions packages/react-devtools-shared/src/backend/renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1021,12 +1021,32 @@ export function attach(
pendingSimulatedUnmountedIDs.length === 0 &&
pendingUnmountedRootID === null
) {
// If we're currently profiling, send an "operations" method even if there are no mutations to the tree.
// The frontend needs this no-op info to know how to reconstruct the tree for each commit,
// even if a particular commit didn't change the shape of the tree.
// If we aren't profiling, we can just bail out here.
// No use sending an empty update over the bridge.
if (!isProfiling) {
return;
}

const current = root.current;
const alternate = current.alternate;

// Certain types of updates bail out at the root without doing any actual render work.
// React should probably not call the DevTools commit hook in this case,
// but if it does- we can detect it and filter them out from the profiler.
// NOTE: Keep this logic in sync with the one in handleCommitFiberRoot()
const didBailoutAtRoot =
alternate !== null &&
alternate.expirationTime === 0 &&
alternate.childExpirationTime === 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

You already know this but this is very fragile and adds a dependency on fields we didn’t use before. I believe these might even be the fields Andrew is currently refactoring.

Can we make a proper fix in React itself instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually never mind, I see we already depend on these fields below. 😳

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, I literally just merged before your comment came in.

The reason I added this check in PR #17253 was because it could also improve the Profiler problem for older versions of React. If we think it's becoming unstable due to @acdlite refactoring, we should remove it and fix it properly in React: see PR #17267


// The Profiler stores metadata for each commit and reconstructs the app tree per commit using:
// (1) an initial tree snapshot and
// (2) the operations array for each commit
// Because of this, it's important that the operations and metadata arrays align,
// So the logic that skips metadata for bailout commits should also apply to filter operations.
if (didBailoutAtRoot) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The added complexity for this bailout sucks. Probably worth it to avoid showing the empty commits in the Profiler and confusing people (see #16980).

return;
}
}

const numUnmountIDs =
Expand Down Expand Up @@ -1758,6 +1778,7 @@ export function attach(
// Certain types of updates bail out at the root without doing any actual render work.
// React should probably not call the DevTools commit hook in this case,
// but if it does- we can detect it and filter them out from the profiler.
// NOTE: Keep this logic in sync with the one in flushPendingEvents()
const didBailoutAtRoot =
alternate !== null &&
alternate.expirationTime === 0 &&
Expand Down