Skip to content

Commit

Permalink
DevTools: Restore inspected element optimizations removed during prev…
Browse files Browse the repository at this point in the history
…ious cache integration

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 bugfix 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.
  • Loading branch information
Brian Vaughn committed Feb 10, 2021
1 parent eee874c commit 476344d
Show file tree
Hide file tree
Showing 12 changed files with 441 additions and 163 deletions.
162 changes: 150 additions & 12 deletions packages/react-devtools-shared/src/__tests__/inspectedElement-test.js
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
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ describe('InspectedElementContext', () => {
const promise = backendAPI
.inspectElement({
bridge,
forceUpdate: true,
id,
inspectedPaths,
rendererID,
Expand Down
8 changes: 3 additions & 5 deletions packages/react-devtools-shared/src/backend/agent.js
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
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
Loading

0 comments on commit 476344d

Please sign in to comment.