Skip to content

Commit

Permalink
DevTools: merge element fields in TreeStateContext (#31956)
Browse files Browse the repository at this point in the history 
Stacked on #31892, see commit on
top.

For some reason, there were 2 fields different fields for essentially
same thing: `selectedElementID` and `inspectedElementID`. Basically, the
change is:
```
selectedElementID -> inspectedElementID
selectedElementIndex -> inspectedElementIndex
```

I have a theory that it was due to previously used async approach around
element inspection, and the whole `InspectedElementView` was wrapped in
`Suspense`.
  • Loading branch information
@hoxyq
hoxyq authored Jan 9, 2025
1 parent 54cfa95 commit d634548
Show file tree
Hide file tree
Showing 12 changed files with 265 additions and 278 deletions.
20 changes: 10 additions & 10 deletions packages/react-devtools-inline/__tests__/__e2e__/components.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,15 +119,15 @@ test.describe('Components', () => {
runOnlyForReactRange('>=16.8');

// Select the first list item in DevTools.
await devToolsUtils.selectElement(page, 'ListItem', 'List\nApp');
await devToolsUtils.selectElement(page, 'ListItem', 'List\nApp', true);

// Then read the inspected values.
const sourceText = await page.evaluate(() => {
const {createTestNameSelector, findAllNodes} = window.REACT_DOM_DEVTOOLS;
const container = document.getElementById('devtools');

const source = findAllNodes(container, [
createTestNameSelector('InspectedElementView-Source'),
createTestNameSelector('InspectedElementView-FormattedSourceString'),
])[0];

return source.innerText;
Expand Down Expand Up @@ -237,35 +237,35 @@ test.describe('Components', () => {
}

await focusComponentSearch();
page.keyboard.insertText('List');
await page.keyboard.insertText('List');
let count = await getComponentSearchResultsCount();
expect(count).toBe('1 | 4');

page.keyboard.insertText('Item');
await page.keyboard.insertText('Item');
count = await getComponentSearchResultsCount();
expect(count).toBe('1 | 3');

page.keyboard.press('Enter');
await page.keyboard.press('Enter');
count = await getComponentSearchResultsCount();
expect(count).toBe('2 | 3');

page.keyboard.press('Enter');
await page.keyboard.press('Enter');
count = await getComponentSearchResultsCount();
expect(count).toBe('3 | 3');

page.keyboard.press('Enter');
await page.keyboard.press('Enter');
count = await getComponentSearchResultsCount();
expect(count).toBe('1 | 3');

page.keyboard.press('Shift+Enter');
await page.keyboard.press('Shift+Enter');
count = await getComponentSearchResultsCount();
expect(count).toBe('3 | 3');

page.keyboard.press('Shift+Enter');
await page.keyboard.press('Shift+Enter');
count = await getComponentSearchResultsCount();
expect(count).toBe('2 | 3');

page.keyboard.press('Shift+Enter');
await page.keyboard.press('Shift+Enter');
count = await getComponentSearchResultsCount();
expect(count).toBe('1 | 3');
});
Expand Down
21 changes: 20 additions & 1 deletion packages/react-devtools-inline/__tests__/__e2e__/devtools-utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,12 @@ async function getElementCount(page, displayName) {
}, displayName);
}

async function selectElement(page, displayName, waitForOwnersText) {
async function selectElement(
page,
displayName,
waitForOwnersText,
waitForSourceLoaded = false
) {
await page.evaluate(listItemText => {
const {createTestNameSelector, createTextSelector, findAllNodes} =
window.REACT_DOM_DEVTOOLS;
Expand Down Expand Up @@ -69,6 +74,20 @@ async function selectElement(page, displayName, waitForOwnersText) {
{titleText: displayName, ownersListText: waitForOwnersText}
);
}

if (waitForSourceLoaded) {
await page.waitForFunction(() => {
const {createTestNameSelector, findAllNodes} = window.REACT_DOM_DEVTOOLS;
const container = document.getElementById('devtools');

const sourceStringBlock = findAllNodes(container, [
createTestNameSelector('InspectedElementView-FormattedSourceString'),
])[0];

// Wait for a new source line to be fetched
return sourceStringBlock != null && sourceStringBlock.innerText != null;
});
}
}

module.exports = {
Expand Down
25 changes: 13 additions & 12 deletions packages/react-devtools-shared/src/__tests__/inspectedElement-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,16 +115,15 @@ describe('InspectedElement', () => {

const Contexts = ({
children,
defaultSelectedElementID = null,
defaultSelectedElementIndex = null,
defaultInspectedElementID = null,
defaultInspectedElementIndex = null,
}) => (
<BridgeContext.Provider value={bridge}>
<StoreContext.Provider value={store}>
<SettingsContextController>
<TreeContextController
defaultSelectedElementID={defaultSelectedElementID}
defaultSelectedElementIndex={defaultSelectedElementIndex}
defaultInspectedElementID={defaultSelectedElementID}>
defaultInspectedElementID={defaultInspectedElementID}
defaultInspectedElementIndex={defaultInspectedElementIndex}>
<InspectedElementContextController>
{children}
</InspectedElementContextController>
Expand Down Expand Up @@ -167,8 +166,8 @@ describe('InspectedElement', () => {
testRendererInstance.update(
<ErrorBoundary>
<Contexts
defaultSelectedElementID={id}
defaultSelectedElementIndex={index}>
defaultInspectedElementID={id}
defaultInspectedElementIndex={index}>
<React.Suspense fallback={null}>
<Suspender id={id} index={index} />
</React.Suspense>
Expand Down Expand Up @@ -355,7 +354,7 @@ describe('InspectedElement', () => {
const {index, shouldHaveLegacyContext} = cases[i];

// HACK: Recreate TestRenderer instance because we rely on default state values
// from props like defaultSelectedElementID and it's easier to reset here than
// from props like defaultInspectedElementID and it's easier to reset here than
// to read the TreeDispatcherContext and update the selected ID that way.
// We're testing the inspected values here, not the context wiring, so that's ok.
withErrorsOrWarningsIgnored(
Expand Down Expand Up @@ -2069,7 +2068,7 @@ describe('InspectedElement', () => {
}, false);

// HACK: Recreate TestRenderer instance because we rely on default state values
// from props like defaultSelectedElementID and it's easier to reset here than
// from props like defaultInspectedElementID and it's easier to reset here than
// to read the TreeDispatcherContext and update the selected ID that way.
// We're testing the inspected values here, not the context wiring, so that's ok.
withErrorsOrWarningsIgnored(
Expand Down Expand Up @@ -2129,7 +2128,7 @@ describe('InspectedElement', () => {
}, false);

// HACK: Recreate TestRenderer instance because we rely on default state values
// from props like defaultSelectedElementID and it's easier to reset here than
// from props like defaultInspectedElementID and it's easier to reset here than
// to read the TreeDispatcherContext and update the selected ID that way.
// We're testing the inspected values here, not the context wiring, so that's ok.
withErrorsOrWarningsIgnored(
Expand Down Expand Up @@ -2408,8 +2407,8 @@ describe('InspectedElement', () => {
await utils.actAsync(() => {
root = TestRenderer.create(
<Contexts
defaultSelectedElementID={id}
defaultSelectedElementIndex={index}>
defaultInspectedElementID={id}
defaultInspectedElementIndex={index}>
<React.Suspense fallback={null}>
<Suspender target={id} />
</React.Suspense>
Expand Down Expand Up @@ -3101,6 +3100,7 @@ describe('InspectedElement', () => {

await utils.actAsync(() => {
store.componentFilters = [utils.createDisplayNameFilter('Wrapper')];
jest.runOnlyPendingTimers();
}, false);

expect(state).toMatchInlineSnapshot(`
Expand All @@ -3120,6 +3120,7 @@ describe('InspectedElement', () => {

await utils.actAsync(() => {
store.componentFilters = [];
jest.runOnlyPendingTimers();
}, false);
expect(state).toMatchInlineSnapshot(`
✕ 0, ⚠ 2
Expand Down
61 changes: 34 additions & 27 deletions packages/react-devtools-shared/src/__tests__/profilerContext-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,14 +69,14 @@ describe('ProfilerContext', () => {

const Contexts = ({
children = null,
defaultSelectedElementID = null,
defaultSelectedElementIndex = null,
defaultInspectedElementID = null,
defaultInspectedElementIndex = null,
}: any) => (
<BridgeContext.Provider value={bridge}>
<StoreContext.Provider value={store}>
<TreeContextController
defaultSelectedElementID={defaultSelectedElementID}
defaultSelectedElementIndex={defaultSelectedElementIndex}>
defaultInspectedElementID={defaultInspectedElementID}
defaultInspectedElementIndex={defaultInspectedElementIndex}>
<ProfilerContextController>{children}</ProfilerContextController>
</TreeContextController>
</StoreContext.Provider>
Expand Down Expand Up @@ -225,8 +225,8 @@ describe('ProfilerContext', () => {
await utils.actAsync(() =>
TestRenderer.create(
<Contexts
defaultSelectedElementID={store.getElementIDAtIndex(3)}
defaultSelectedElementIndex={3}>
defaultInspectedElementID={store.getElementIDAtIndex(3)}
defaultInspectedElementIndex={3}>
<ContextReader />
</Contexts>,
),
Expand Down Expand Up @@ -276,8 +276,8 @@ describe('ProfilerContext', () => {
await utils.actAsync(() =>
TestRenderer.create(
<Contexts
defaultSelectedElementID={store.getElementIDAtIndex(3)}
defaultSelectedElementIndex={3}>
defaultInspectedElementID={store.getElementIDAtIndex(3)}
defaultInspectedElementIndex={3}>
<ContextReader />
</Contexts>,
),
Expand Down Expand Up @@ -323,8 +323,8 @@ describe('ProfilerContext', () => {
await utils.actAsync(() =>
TestRenderer.create(
<Contexts
defaultSelectedElementID={store.getElementIDAtIndex(3)}
defaultSelectedElementIndex={3}>
defaultInspectedElementID={store.getElementIDAtIndex(3)}
defaultInspectedElementIndex={3}>
<ContextReader />
</Contexts>,
),
Expand Down Expand Up @@ -374,8 +374,8 @@ describe('ProfilerContext', () => {
await utils.actAsync(() =>
TestRenderer.create(
<Contexts
defaultSelectedElementID={store.getElementIDAtIndex(3)}
defaultSelectedElementIndex={3}>
defaultInspectedElementID={store.getElementIDAtIndex(3)}
defaultInspectedElementIndex={3}>
<ContextReader />
</Contexts>,
),
Expand Down Expand Up @@ -415,11 +415,12 @@ describe('ProfilerContext', () => {

let context: Context = ((null: any): Context);
let dispatch: DispatcherContext = ((null: any): DispatcherContext);
let selectedElementID = null;
let inspectedElementID = null;
function ContextReader() {
context = React.useContext(ProfilerContext);
dispatch = React.useContext(TreeDispatcherContext);
selectedElementID = React.useContext(TreeStateContext).selectedElementID;
inspectedElementID =
React.useContext(TreeStateContext).inspectedElementID;
return null;
}

Expand All @@ -428,13 +429,15 @@ describe('ProfilerContext', () => {
// Select an element within the second root.
await utils.actAsync(() =>
TestRenderer.create(
<Contexts defaultSelectedElementID={id} defaultSelectedElementIndex={3}>
<Contexts
defaultInspectedElementID={id}
defaultInspectedElementIndex={3}>
<ContextReader />
</Contexts>,
),
);

expect(selectedElementID).toBe(id);
expect(inspectedElementID).toBe(id);

// Profile and record more updates to both roots
await utils.actAsync(() => store.profilerStore.startProfiling());
Expand All @@ -448,7 +451,7 @@ describe('ProfilerContext', () => {
utils.act(() => dispatch({type: 'SELECT_ELEMENT_AT_INDEX', payload: 0}));

// Verify that the initial Profiler root selection is maintained.
expect(selectedElementID).toBe(otherID);
expect(inspectedElementID).toBe(otherID);
expect(context).not.toBeNull();
expect(context.rootID).toBe(store.getRootIDForElement(id));
});
Expand Down Expand Up @@ -484,11 +487,12 @@ describe('ProfilerContext', () => {

let context: Context = ((null: any): Context);
let dispatch: DispatcherContext = ((null: any): DispatcherContext);
let selectedElementID = null;
let inspectedElementID = null;
function ContextReader() {
context = React.useContext(ProfilerContext);
dispatch = React.useContext(TreeDispatcherContext);
selectedElementID = React.useContext(TreeStateContext).selectedElementID;
inspectedElementID =
React.useContext(TreeStateContext).inspectedElementID;
return null;
}

Expand All @@ -497,13 +501,15 @@ describe('ProfilerContext', () => {
// Select an element within the second root.
await utils.actAsync(() =>
TestRenderer.create(
<Contexts defaultSelectedElementID={id} defaultSelectedElementIndex={3}>
<Contexts
defaultInspectedElementID={id}
defaultInspectedElementIndex={3}>
<ContextReader />
</Contexts>,
),
);

expect(selectedElementID).toBe(id);
expect(inspectedElementID).toBe(id);

// Profile and record more updates to both roots
await utils.actAsync(() => store.profilerStore.startProfiling());
Expand All @@ -517,7 +523,7 @@ describe('ProfilerContext', () => {
utils.act(() => dispatch({type: 'SELECT_ELEMENT_AT_INDEX', payload: 0}));

// Verify that the initial Profiler root selection is maintained.
expect(selectedElementID).toBe(otherID);
expect(inspectedElementID).toBe(otherID);
expect(context).not.toBeNull();
expect(context.rootID).toBe(store.getRootIDForElement(id));
});
Expand Down Expand Up @@ -553,10 +559,11 @@ describe('ProfilerContext', () => {
`);

let context: Context = ((null: any): Context);
let selectedElementID = null;
let inspectedElementID = null;
function ContextReader() {
context = React.useContext(ProfilerContext);
selectedElementID = React.useContext(TreeStateContext).selectedElementID;
inspectedElementID =
React.useContext(TreeStateContext).inspectedElementID;
return null;
}

Expand All @@ -567,14 +574,14 @@ describe('ProfilerContext', () => {
</Contexts>,
),
);
expect(selectedElementID).toBeNull();
expect(inspectedElementID).toBeNull();

// Select an element in the Profiler tab and verify that the selection is synced to the Components tab.
await utils.actAsync(() => context.selectFiber(parentID, 'Parent'));
expect(selectedElementID).toBe(parentID);
expect(inspectedElementID).toBe(parentID);

// 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(inspectedElementID).toBe(parentID);
});
});
2 changes: 1 addition & 1 deletion packages/react-devtools-shared/src/devtools/utils.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ export function printStore(
if (state === null) {
return '';
}
return state.selectedElementIndex === index ? `→` : ' ';
return state.inspectedElementIndex === index ? `→` : ' ';
}

function printErrorsAndWarnings(element: Element): string {
Expand Down
4 changes: 2 additions & 2 deletions packages/react-devtools-shared/src/devtools/views/Components/Element.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ type Props = {

export default function Element({data, index, style}: Props): React.Node {
const store = useContext(StoreContext);
const {ownerFlatTree, ownerID, selectedElementID} =
const {ownerFlatTree, ownerID, inspectedElementID} =
useContext(TreeStateContext);
const dispatch = useContext(TreeDispatcherContext);

Expand All @@ -46,7 +46,7 @@ export default function Element({data, index, style}: Props): React.Node {

const {isNavigatingWithKeyboard, onElementMouseEnter, treeFocused} = data;
const id = element === null ? null : element.id;
const isSelected = selectedElementID === id;
const isSelected = inspectedElementID === id;

const errorsAndWarningsSubscription = useMemo(
() => ({
Expand Down
Loading

0 comments on commit d634548

Please sign in to comment.