Skip to content

Commit

Permalink
DevTools: Restore inspect-element bridge optimizations (#20789)
Browse files Browse the repository at this point in the history 
* Restore inspect-element bridge optimizations

When the new Suspense cache was integrated (so that startTransition could be used) I removed a couple of optimizations between the backend and frontend that reduced bridge traffic when e.g. dehydrated paths were inspected for elements that had not rendered since previously inspected. This commit re-adds those optimizations as well as an additional test with a bug fix that I noticed while reading the backend code.

There are two remaining TODO items as of this commit:
- Make inspected element edits and deletes also use transition API
- Don't over-eagerly refresh the cache in our ping-for-updates handler

I will addres both in subsequent commits.

* Poll for update only refreshes cache when there's an update

* Added inline comment
  • Loading branch information
Brian Vaughn committed Feb 22, 2021
1 parent 4d28eca commit cfd8c1b
Show file tree
Hide file tree
Showing 13 changed files with 475 additions and 170 deletions.
162 changes: 150 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 @@ -65,7 +65,9 @@ describe('InspectedElement', () => {
.TreeContextController;

// Used by inspectElementAtIndex() helper function
testRendererInstance = TestRenderer.create(null);
testRendererInstance = TestRenderer.create(null, {
unstable_isConcurrent: true,
});
});

afterEach(() => {
Expand Down Expand Up @@ -259,7 +261,9 @@ describe('InspectedElement', () => {
// from props like defaultSelectedElementID 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.
testRendererInstance = TestRenderer.create(null);
testRendererInstance = TestRenderer.create(null, {
unstable_isConcurrent: true,
});

const inspectedElement = await inspectElementAtIndex(index);

Expand Down Expand Up @@ -1197,10 +1201,9 @@ describe('InspectedElement', () => {
"1": 2,
"2": 3,
},
"1": Object {
"0": "a",
"1": "b",
"2": "c",
"1": Dehydrated {
"preview_short": Set(3),
"preview_long": Set(3) {"a", "b", "c"},
},
},
}
Expand Down Expand Up @@ -1283,12 +1286,9 @@ describe('InspectedElement', () => {
},
"value": 1,
},
"c": Object {
"d": Dehydrated {
"preview_short": {…},
"preview_long": {e: {…}, value: 1},
},
"value": 1,
"c": Dehydrated {
"preview_short": {…},
"preview_long": {d: {…}, value: 1},
},
},
}
Expand Down Expand Up @@ -1377,6 +1377,143 @@ describe('InspectedElement', () => {
done();
});

it('should return a full update if a path is inspected for an object that has other pending changes', async done => {
const Example = () => null;

const container = document.createElement('div');
await utils.actAsync(() =>
ReactDOM.render(
<Example
nestedObject={{
a: {
value: 1,
b: {
value: 1,
},
},
c: {
value: 1,
d: {
value: 1,
e: {
value: 1,
},
},
},
}}
/>,
container,
),
);

let inspectedElement = null;
let inspectElementPath = null;

// Render once to get a handle on inspectElementPath()
inspectedElement = await inspectElementAtIndex(0, () => {
inspectElementPath = useInspectElementPath();
});

async function loadPath(path) {
TestUtilsAct(() => {
TestRendererAct(() => {
inspectElementPath(path);
jest.runOnlyPendingTimers();
});
});

inspectedElement = await inspectElementAtIndex(0);
}

expect(inspectedElement.props).toMatchInlineSnapshot(`
Object {
"nestedObject": Object {
"a": Dehydrated {
"preview_short": {…},
"preview_long": {b: {…}, value: 1},
},
"c": Dehydrated {
"preview_short": {…},
"preview_long": {d: {…}, value: 1},
},
},
}
`);

await loadPath(['props', 'nestedObject', 'a']);

expect(inspectedElement.props).toMatchInlineSnapshot(`
Object {
"nestedObject": Object {
"a": Object {
"b": Object {
"value": 1,
},
"value": 1,
},
"c": Dehydrated {
"preview_short": {…},
"preview_long": {d: {…}, value: 1},
},
},
}
`);

TestRendererAct(() => {
TestUtilsAct(() => {
ReactDOM.render(
<Example
nestedObject={{
a: {
value: 2,
b: {
value: 2,
},
},
c: {
value: 2,
d: {
value: 2,
e: {
value: 2,
},
},
},
}}
/>,
container,
);
});
});

await loadPath(['props', 'nestedObject', 'c']);

expect(inspectedElement.props).toMatchInlineSnapshot(`
Object {
"nestedObject": Object {
"a": Object {
"b": Object {
"value": 2,
},
"value": 2,
},
"c": Object {
"d": Object {
"e": Dehydrated {
"preview_short": {…},
"preview_long": {value: 2},
},
"value": 2,
},
"value": 2,
},
},
}
`);

done();
});

it('should not tear if hydration is requested after an update', async done => {
const Example = () => null;

Expand Down Expand Up @@ -1936,6 +2073,7 @@ describe('InspectedElement', () => {
<Suspender target={id} />
</React.Suspense>
</Contexts>,
{unstable_isConcurrent: true},
);
}, false);
await utils.actAsync(() => {
Expand Down
23 changes: 13 additions & 10 deletions packages/react-devtools-shared/src/__tests__/legacy/inspectElement-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,14 @@ describe('InspectedElementContext', () => {

async function read(
id: number,
inspectedPaths?: Object = {},
path?: Array<string | number> = null,
): Promise<Object> {
const rendererID = ((store.getRendererIDForElement(id): any): number);
const promise = backendAPI
.inspectElement({
bridge,
forceUpdate: true,
id,
inspectedPaths,
path,
rendererID,
})
.then(data =>
Expand Down Expand Up @@ -686,7 +685,7 @@ describe('InspectedElementContext', () => {
}
`);

inspectedElement = await read(id, {props: {nestedObject: {a: {}}}});
inspectedElement = await read(id, ['props', 'nestedObject', 'a']);
expect(inspectedElement.props).toMatchInlineSnapshot(`
Object {
"nestedObject": Object {
Expand All @@ -702,9 +701,7 @@ describe('InspectedElementContext', () => {
}
`);

inspectedElement = await read(id, {
props: {nestedObject: {a: {b: {c: {}}}}},
});
inspectedElement = await read(id, ['props', 'nestedObject', 'a', 'b', 'c']);
expect(inspectedElement.props).toMatchInlineSnapshot(`
Object {
"nestedObject": Object {
Expand All @@ -724,9 +721,15 @@ describe('InspectedElementContext', () => {
}
`);

inspectedElement = await read(id, {
props: {nestedObject: {a: {b: {c: {0: {d: {}}}}}}},
});
inspectedElement = await read(id, [
'props',
'nestedObject',
'a',
'b',
'c',
0,
'd',
]);
expect(inspectedElement.props).toMatchInlineSnapshot(`
Object {
"nestedObject": Object {
Expand Down
8 changes: 3 additions & 5 deletions packages/react-devtools-shared/src/backend/agent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,7 @@ type CopyElementParams = {|

type InspectElementParams = {|
id: number,
inspectedPaths: Object,
forceUpdate: boolean,
path: Array<string | number> | null,
rendererID: number,
requestID: number,
|};
Expand Down Expand Up @@ -332,8 +331,7 @@ export default class Agent extends EventEmitter<{|

inspectElement = ({
id,
inspectedPaths,
forceUpdate,
path,
rendererID,
requestID,
}: InspectElementParams) => {
Expand All @@ -343,7 +341,7 @@ export default class Agent extends EventEmitter<{|
} else {
this._bridge.send(
'inspectedElement',
renderer.inspectElement(requestID, id, inspectedPaths, forceUpdate),
renderer.inspectElement(requestID, id, path),
);

// When user selects an element, stop trying to restore the selection,
Expand Down
30 changes: 24 additions & 6 deletions packages/react-devtools-shared/src/backend/legacy/renderer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -584,12 +584,25 @@ export function attach(
}

let currentlyInspectedElementID: number | null = null;
let currentlyInspectedPaths: Object = {};

// Track the intersection of currently inspected paths,
// so that we can send their data along if the element is re-rendered.
function mergeInspectedPaths(path: Array<string | number>) {
let current = currentlyInspectedPaths;
path.forEach(key => {
if (!current[key]) {
current[key] = {};
}
current = current[key];
});
}

function createIsPathAllowed(key: string, inspectedPaths: Object) {
function createIsPathAllowed(key: string) {
// This function helps prevent previously-inspected paths from being dehydrated in updates.
// This is important to avoid a bad user experience where expanded toggles collapse on update.
return function isPathAllowed(path: Array<string | number>): boolean {
let current = inspectedPaths[key];
let current = currentlyInspectedPaths[key];
if (!current) {
return false;
}
Expand Down Expand Up @@ -680,10 +693,11 @@ export function attach(
function inspectElement(
requestID: number,
id: number,
inspectedPaths: Object,
path: Array<string | number> | null,
): InspectedElementPayload {
if (currentlyInspectedElementID !== id) {
currentlyInspectedElementID = id;
currentlyInspectedPaths = {};
}

const inspectedElement = inspectElementRaw(id);
Expand All @@ -695,22 +709,26 @@ export function attach(
};
}

if (path !== null) {
mergeInspectedPaths(path);
}

// Any time an inspected element has an update,
// we should update the selected $r value as wel.
// Do this before dehyration (cleanForBridge).
updateSelectedElement(id);

inspectedElement.context = cleanForBridge(
inspectedElement.context,
createIsPathAllowed('context', inspectedPaths),
createIsPathAllowed('context'),
);
inspectedElement.props = cleanForBridge(
inspectedElement.props,
createIsPathAllowed('props', inspectedPaths),
createIsPathAllowed('props'),
);
inspectedElement.state = cleanForBridge(
inspectedElement.state,
createIsPathAllowed('state', inspectedPaths),
createIsPathAllowed('state'),
);

return {
Expand Down

0 comments on commit cfd8c1b

Please sign in to comment.