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.

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

Profiler: Skip reading element for imported data #18913

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Expand Up @@ -337,18 +337,10 @@ describe('ProfilerContext', () => {
await utils.actAsync(() => context.selectFiber(parentID, 'Parent'));
expect(selectedElementID).toBe(parentID);

// We expect a "no element found" warning.
// Let's hide it from the test console though.
spyOn(console, 'warn');

// Select an unmounted element and verify no Components tab selection doesn't change.
await utils.actAsync(() => context.selectFiber(childID, 'Child'));
expect(selectedElementID).toBe(parentID);

expect(console.warn).toHaveBeenCalledWith(
`No element found with id "${childID}"`,
);

done();
});
});
8 changes: 6 additions & 2 deletions packages/react-devtools-shared/src/__tests__/utils.js
Expand Up @@ -181,6 +181,7 @@ export function exportImportHelper(bridge: FrontendBridge, store: Store): void {
expect(profilerStore.profilingData).not.toBeNull();

const profilingDataFrontendInitial = ((profilerStore.profilingData: any): ProfilingDataFrontend);
expect(profilingDataFrontendInitial.imported).toBe(false);

const profilingDataExport = prepareProfilingDataExport(
profilingDataFrontendInitial,
Expand All @@ -197,15 +198,18 @@ export function exportImportHelper(bridge: FrontendBridge, store: Store): void {
const profilingDataFrontend = prepareProfilingDataFrontendFromExport(
(parsedProfilingDataExport: any),
);
expect(profilingDataFrontend.imported).toBe(true);

// Sanity check that profiling snapshots are serialized correctly.
expect(profilingDataFrontendInitial).toEqual(profilingDataFrontend);
expect(profilingDataFrontendInitial.dataForRoots).toEqual(
profilingDataFrontend.dataForRoots,
);

// Snapshot the JSON-parsed object, rather than the raw string, because Jest formats the diff nicer.
expect(parsedProfilingDataExport).toMatchSnapshot('imported data');

act(() => {
// Apply the new exported-then-reimported data so tests can re-run assertions.
// Apply the new exported-then-imported data so tests can re-run assertions.
profilerStore.profilingData = profilingDataFrontend;
});
}
Expand Up @@ -138,20 +138,24 @@ function ProfilerContextController({children}: Props) {
selectFiberName(name);

// Sync selection to the Components tab for convenience.
if (id !== null) {
const element = store.getElementByID(id);

// Keep in mind that profiling data may be from a previous session.
// In that case, IDs may match up arbitrarily; to be safe, compare both ID and display name.
if (element !== null && element.displayName === name) {
// Keep in mind that profiling data may be from a previous session.
// If data has been imported, we should skip the selection sync.
if (
id !== null &&
profilingData !== null &&
profilingData.imported === false
) {
// We should still check to see if this element is still in the store.
// It may have been removed during profiling.
if (store.containsElement(id)) {
dispatch({
type: 'SELECT_ELEMENT_BY_ID',
payload: id,
});
}
}
},
[dispatch, selectFiberID, selectFiberName, store],
[dispatch, selectFiberID, selectFiberName, store, profilingData],
);

const setRootIDAndClearFiber = useCallback(
Expand Down
Expand Up @@ -105,6 +105,7 @@ export type ProfilingDataForRootFrontend = {|
export type ProfilingDataFrontend = {|
// Profiling data per root.
dataForRoots: Map<number, ProfilingDataForRootFrontend>,
imported: boolean,
|};

export type CommitDataExport = {|
Expand Down
Expand Up @@ -99,7 +99,7 @@ export function prepareProfilingDataFrontendFromBackendAndStore(
);
});

return {dataForRoots};
return {dataForRoots, imported: false};
}

// Converts a Profiling data export into the format required by the Store.
Expand Down Expand Up @@ -156,7 +156,7 @@ export function prepareProfilingDataFrontendFromExport(
},
);

return {dataForRoots};
return {dataForRoots, imported: true};
}

// Converts a Store Profiling data into a format that can be safely (JSON) serialized for export.
Expand Down