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

Fix Profiler root change error #18880

Merged
merged 3 commits into from
May 12, 2020
Merged

Fix Profiler root change error #18880

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
726e89b
fix profiler root change error
bl00mber May 9, 2020
d840ca5
always clean fiber
bl00mber May 10, 2020
aaf40f4
change name
bl00mber May 12, 2020
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
68 changes: 38 additions & 30 deletions packages/react-devtools-shared/src/devtools/views/Profiler/ProfilerContext.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import type {ProfilingDataFrontend} from './types';
export type TabID = 'flame-chart' | 'ranked-chart' | 'interactions';

export type Context = {|
// Which tab is selexted in the Profiler UI?
// Which tab is selected in the Profiler UI?
selectedTabID: TabID,
selectTab(id: TabID): void,

Expand All @@ -43,7 +43,7 @@ export type Context = {|
// 1. The selected root in the Components tree (if it has any profiling data) or
// 2. The first root in the list with profiling data.
rootID: number | null,
setRootID: (id: number) => void,
setRootIDcleanFiber: (id: number) => void,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think setRootIDcleanFiber is kind of an awkward name. Prefer to just leave the name we pass down through the context as setRootID (and change the implementation to also clear the fiber ID like you did)


// Controls whether commits are filtered by duration.
// This value is controlled by a filter toggle UI in the Profiler toolbar.
Expand Down Expand Up @@ -129,6 +129,38 @@ function ProfilerContextController({children}: Props) {
setPrevProfilingData,
] = useState<ProfilingDataFrontend | null>(null);
const [rootID, setRootID] = useState<number | null>(null);
const [selectedFiberID, selectFiberID] = useState<number | null>(null);
const [selectedFiberName, selectFiberName] = useState<string | null>(null);

const selectFiber = useCallback(
(id: number | null, name: string | null) => {
selectFiberID(id);
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, right. So this seems to come up in a few places. I know this check was copied from pre-existing code, but looking at it again- the heuristic is flawed. ID and name might still happen to match for imported data.

We should check something on the data itself to show that it was imported or not, and just skip looking in the store for such imported elements.

We could add a new field to the ProfilingDataFrontend type to track this. Interested in tackling this? (Totally fine if you aren't, I could do it.)

Copy link
Contributor Author

@bl00mber bl00mber May 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, it could be interesting to do. Is all of the task description in this comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure I understand?

I'm just thinking we might add a flag to the ProfilingDataFrontend that specifies whether it was imported (from a previous session) then use that flag to determine if we should disable certain features (like reading element data from the Store).

Copy link
Contributor

@bvaughn bvaughn May 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Just to be clear, this doesn't need to be done- at least not as part of this PR. I could do it as a follow up (or you could if you're interested).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll try to do it in the next few days, thanks!

Copy link
Contributor Author

@bl00mber bl00mber May 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

#18913

if (element !== null && element.displayName === name) {
dispatch({
type: 'SELECT_ELEMENT_BY_ID',
payload: id,
});
}
}
},
[dispatch, selectFiberID, selectFiberName, store],
);

const setRootIDcleanFiber = useCallback(
(id: number | null) => {
selectFiber(null, null);
setRootID(id);
},
[setRootID, selectFiber],
);

if (prevProfilingData !== profilingData) {
batchedUpdates(() => {
Expand All @@ -150,9 +182,9 @@ function ProfilerContextController({children}: Props) {
selectedElementRootID !== null &&
dataForRoots.has(selectedElementRootID)
) {
setRootID(selectedElementRootID);
setRootIDcleanFiber(selectedElementRootID);
} else {
setRootID(firstRootID);
setRootIDcleanFiber(firstRootID);
}
}
}
Expand Down Expand Up @@ -180,34 +212,10 @@ function ProfilerContextController({children}: Props) {
null,
);
const [selectedTabID, selectTab] = useState<TabID>('flame-chart');
const [selectedFiberID, selectFiberID] = useState<number | null>(null);
const [selectedFiberName, selectFiberName] = useState<string | null>(null);
const [selectedInteractionID, selectInteraction] = useState<number | null>(
null,
);

const selectFiber = useCallback(
(id: number | null, name: string | null) => {
selectFiberID(id);
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) {
dispatch({
type: 'SELECT_ELEMENT_BY_ID',
payload: id,
});
}
}
},
[dispatch, selectFiberID, selectFiberName, store],
);

if (isProfiling) {
batchedUpdates(() => {
if (selectedCommitIndex !== null) {
Expand Down Expand Up @@ -237,7 +245,7 @@ function ProfilerContextController({children}: Props) {
supportsProfiling,

rootID,
setRootID,
setRootIDcleanFiber,
Copy link
Contributor

Choose a reason for hiding this comment

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

In other words, setRootID: setRootIDAndClearFiber


isCommitFilterEnabled,
setIsCommitFilterEnabled,
Expand Down Expand Up @@ -267,7 +275,7 @@ function ProfilerContextController({children}: Props) {
supportsProfiling,

rootID,
setRootID,
setRootIDcleanFiber,

isCommitFilterEnabled,
setIsCommitFilterEnabled,
Expand Down
8 changes: 5 additions & 3 deletions packages/react-devtools-shared/src/devtools/views/Profiler/RootSelector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@ import {ProfilerContext} from './ProfilerContext';
import styles from './RootSelector.css';

export default function RootSelector(_: {||}) {
const {profilingData, rootID, setRootID} = useContext(ProfilerContext);
const {profilingData, rootID, setRootIDcleanFiber} = useContext(
ProfilerContext,
);

const options = [];
if (profilingData !== null) {
Expand All @@ -29,9 +31,9 @@ export default function RootSelector(_: {||}) {

const handleChange = useCallback(
({currentTarget}) => {
setRootID(parseInt(currentTarget.value, 10));
setRootIDcleanFiber(parseInt(currentTarget.value, 10));
},
[setRootID],
[setRootIDcleanFiber],
);

if (profilingData === null || profilingData.dataForRoots.size <= 1) {
Expand Down