Skip to content
This repository was archived by the owner on Sep 21, 2021. It is now read-only.

Commit 3f5162b

Browse files
committed
Do not create an ObjectInspector element for primitive grips.
When we have a single primitive root, there's no advantage rendering them inside the ObjectInspector component. On the contrary, it adds some overhead that can harm performances. This patch allow us to branch early so we don't go through the ObjectInspector element if we don't need to.
1 parent 72fd3a8 commit 3f5162b

File tree

5 files changed

+126
-33
lines changed

5 files changed

+126
-33
lines changed

packages/devtools-reps/src/object-inspector/component.js

Lines changed: 2 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,6 @@ const Tree = createFactory(Components.Tree);
1616
require("./index.css");
1717

1818
const classnames = require("classnames");
19-
20-
const {
21-
REPS: {
22-
Rep,
23-
Grip,
24-
},
25-
} = require("../reps/rep");
2619
const {
2720
MODE,
2821
} = require("../reps/constants");
@@ -265,7 +258,7 @@ class ObjectInspector extends Component {
265258
)
266259
) {
267260
return {
268-
label: this.renderGrip(
261+
label: Utils.renderRep(
269262
item,
270263
{
271264
...this.props,
@@ -293,7 +286,7 @@ class ObjectInspector extends Component {
293286

294287
return {
295288
label,
296-
value: this.renderGrip(item, repsProp)
289+
value: Utils.renderRep(item, repsProp)
297290
};
298291
}
299292

@@ -412,19 +405,6 @@ class ObjectInspector extends Component {
412405
);
413406
}
414407

415-
renderGrip(
416-
item: Node,
417-
props: Props
418-
) {
419-
const object = getValue(item);
420-
return Rep({
421-
...props,
422-
object,
423-
mode: props.mode || MODE.TINY,
424-
defaultRep: Grip,
425-
});
426-
}
427-
428408
render() {
429409
const {
430410
autoExpandAll = true,
@@ -436,15 +416,6 @@ class ObjectInspector extends Component {
436416
inline,
437417
} = this.props;
438418

439-
let roots = this.getRoots();
440-
if (roots.length === 1) {
441-
const root = roots[0];
442-
const name = root && root.name;
443-
if (nodeIsPrimitive(root) && (name === null || typeof name === "undefined")) {
444-
return this.renderGrip(root, this.props);
445-
}
446-
}
447-
448419
return Tree({
449420
className: classnames({
450421
inline,

packages/devtools-reps/src/object-inspector/index.js

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,11 @@ const { createElement, createFactory, PureComponent } = require("react");
77
const { Provider } = require("react-redux");
88
const ObjectInspector = createFactory(require("./component"));
99
const createStore = require("./store");
10+
const Utils = require("./utils");
11+
const {
12+
renderRep,
13+
shouldRenderRootsInReps
14+
} = Utils;
1015

1116
import type { Props, State } from "./types";
1217

@@ -32,4 +37,10 @@ class OI extends PureComponent {
3237
}
3338
}
3439

35-
module.exports = OI;
40+
module.exports = (props: Props) => {
41+
let {roots} = props;
42+
if (shouldRenderRootsInReps(roots)) {
43+
return renderRep(roots[0], props);
44+
}
45+
return new OI(props);
46+
};

packages/devtools-reps/src/object-inspector/tests/component/basic.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ describe("ObjectInspector - renders", () => {
6363
expect(formatObjectInspector(oi)).toMatchSnapshot();
6464
});
6565

66-
it("directly renders a Rep with the stub is not expandable", () => {
66+
it("directly renders a Rep when the stub is not expandable", () => {
6767
const object = 42;
6868

6969
const renderObjectInspector = mode =>
Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
/* This Source Code Form is subject to the terms of the Mozilla Public
2+
* License, v. 2.0. If a copy of the MPL was not distributed with this
3+
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
4+
5+
const Utils = require("../../utils");
6+
const { shouldRenderRootsInReps } = Utils;
7+
8+
const nullStubs = require("../../../reps/stubs/null");
9+
const numberStubs = require("../../../reps/stubs/number");
10+
const undefinedStubs = require("../../../reps/stubs/undefined");
11+
const gripStubs = require("../../../reps/stubs/grip");
12+
const gripArrayStubs = require("../../../reps/stubs/grip-array");
13+
const symbolStubs = require("../../../reps/stubs/symbol");
14+
15+
describe("shouldRenderRootsInReps", () => {
16+
it("returns true for a string", () => {
17+
expect(shouldRenderRootsInReps([{
18+
contents: { value: "Hello" }
19+
}])).toBeTruthy();
20+
});
21+
22+
it("returns true for an integer", () => {
23+
expect(shouldRenderRootsInReps([{
24+
contents: { value: numberStubs.get("Int") }
25+
}])).toBeTruthy();
26+
});
27+
28+
it("returns true for undefined", () => {
29+
expect(shouldRenderRootsInReps([{
30+
contents: { value: undefinedStubs.get("Undefined") }
31+
}])).toBeTruthy();
32+
});
33+
34+
it("returns true for null", () => {
35+
expect(shouldRenderRootsInReps([{
36+
contents: { value: nullStubs.get("Null") }
37+
}])).toBeTruthy();
38+
});
39+
40+
it("returns true for Symbols", () => {
41+
expect(shouldRenderRootsInReps([{
42+
contents: { value: symbolStubs.get("Symbol") }
43+
}])).toBeTruthy();
44+
});
45+
46+
it("returns false when there are multiple primitive roots", () => {
47+
expect(shouldRenderRootsInReps([{
48+
contents: { value: "Hello" }
49+
}, {
50+
contents: { value: 42 }
51+
}])).toBeFalsy();
52+
});
53+
54+
it("returns false for primitive when the root specifies a name", () => {
55+
expect(shouldRenderRootsInReps([{
56+
name: "label",
57+
contents: {value: 42}
58+
}])).toBeFalsy();
59+
});
60+
61+
it("returns false for Grips", () => {
62+
expect(shouldRenderRootsInReps([{
63+
name: "label",
64+
contents: {value: gripStubs.get("testMaxProps")}
65+
}])).toBeFalsy();
66+
});
67+
68+
it("returns false for Arrays", () => {
69+
expect(shouldRenderRootsInReps([{
70+
name: "label",
71+
contents: {value: gripArrayStubs.get("testMaxProps")}
72+
}])).toBeFalsy();
73+
});
74+
});

packages/devtools-reps/src/object-inspector/utils/index.js

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,51 @@
22
* License, v. 2.0. If a copy of the MPL was not distributed with this
33
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
44

5+
// @flow
6+
57
const client = require("./client");
68
const loadProperties = require("./load-properties");
79
const node = require("./node");
10+
const { nodeIsPrimitive } = node;
811
const selection = require("./selection");
912

13+
const { MODE } = require("../../reps/constants");
14+
const {
15+
REPS: {
16+
Rep,
17+
Grip,
18+
},
19+
} = require("../../reps/rep");
20+
import type { Node, Props } from "../types";
21+
22+
function shouldRenderRootsInReps(roots: Array<Node>) : boolean {
23+
if (roots.length > 1) {
24+
return false;
25+
}
26+
27+
const root = roots[0];
28+
const name = root && root.name;
29+
return nodeIsPrimitive(root)
30+
&& (name === null || typeof name === "undefined");
31+
}
32+
33+
function renderRep(
34+
item: Node,
35+
props: Props
36+
) {
37+
return Rep({
38+
...props,
39+
object: node.getValue(item),
40+
mode: props.mode || MODE.TINY,
41+
defaultRep: Grip,
42+
});
43+
}
44+
1045
module.exports = {
1146
client,
1247
loadProperties,
1348
node,
49+
renderRep,
1450
selection,
51+
shouldRenderRootsInReps,
1552
};

0 commit comments

Comments
 (0)