Don't add root actors to the state of the ObjectInspector.

The actors property on the ObjectInspector state keep track of all the actors that need to be
released when we're done with the component. The problem of releasing the root actor is that
if the comsumer of the OI was simply hidden, and then shown again, the user would not be
able to expand the objectInspector.
Here we don't track root actors so the consumer handle the lifetime of the root actors it handles.
This causes no issues for non-root actors, since the ObjectInspector is unmounted, if the user
want to expand it again after a hide/show cycle again, we'll end up creating new ObjectClient (and
actors thus).

Author: nchevobbe

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

@@ -315,8 +315,15 @@ class ObjectInspector extends Component {
           const nextLoading = new Map(prevState.loading);
           nextLoading.delete(path);
 
+          const isRoot = this.props.roots.some(root => {
+            const rootValue = getValue(root);
+            return rootValue && rootValue.actor === value.actor;
+          });
+
           return {
-            actors: (new Set(prevState.actors)).add(value.actor),
+            actors: isRoot
+              ? prevState.actors
+              : (new Set(prevState.actors)).add(value.actor),
             loadedProperties: (new Map(prevState.loadedProperties)).set(path, response),
             loading: nextLoading,
           };

packages/devtools-reps/src/object-inspector/tests/component/__snapshots__/state.js.snap

@@ -42,7 +42,14 @@ exports[`ObjectInspector - state has the expected state when expanding a node 1`
 
 exports[`ObjectInspector - state has the expected state when expanding a node 2`] = `
 "▼ {…}
-|    __proto__: Object {  }
+|  ▶︎ __proto__: Object {  }
+▶︎ Proxy { <target>: {…}, <handler>: […] }"
+`;
+
+exports[`ObjectInspector - state has the expected state when expanding a node 3`] = `
+"▼ {…}
+|  ▼ __proto__: {}
+|  |  ▶︎ __proto__: Object {  }
 ▶︎ Proxy { <target>: {…}, <handler>: […] }"
 `;
 

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

@@ -11,9 +11,9 @@ const ObjectInspector = createFactory(require("../../index"));
 const repsPath = "../../../reps";
 const gripRepStubs = require(`${repsPath}/stubs/grip`);
 const ObjectClient = require("../__mocks__/object-client");
+const stub = gripRepStubs.get("testMoreThanMaxProps");
 
 function generateDefaults(overrides) {
-  const stub = gripRepStubs.get("testMoreThanMaxProps");
   return Object.assign({
     autoExpandDepth: 0,
     roots: [{
@@ -29,7 +29,6 @@ function generateDefaults(overrides) {
 describe("release actors", () => {
   it("release actors when unmount", () => {
     const releaseActor = jest.fn();
-    // The window node should have the "lessen" class when collapsed.
     const props = generateDefaults({
       releaseActor,
     });

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

@@ -85,7 +85,19 @@ describe("ObjectInspector - state", () => {
   });
 
   it("has the expected state when expanding a node", async () => {
-    const wrapper = mount(ObjectInspector(generateDefaults({})));
+    const protoStub = {
+      "prototype": {
+        "type": "object",
+        "actor": "server2.conn0.child1/obj628",
+        "class": "Object",
+      }
+    };
+
+    const wrapper = mount(ObjectInspector(generateDefaults({
+      createObjectClient: grip => ObjectClient(grip, {
+        getPrototype: () => Promise.resolve(protoStub)
+      }),
+    })));
     expect(formatObjectInspector(wrapper)).toMatchSnapshot();
     let nodes = wrapper.find(".node");
 
@@ -104,9 +116,28 @@ describe("ObjectInspector - state", () => {
     state = wrapper.state();
     expect(state.loading.has("root-1")).toBeFalsy();
     expect(state.loadedProperties.has("root-1")).toBeTruthy();
+    // We don't want to track root actors.
     expect(state.actors.has(gripRepStubs.get("testMoreThanMaxProps").actor))
-      .toBeTruthy();
+      .toBeFalsy();
     expect(formatObjectInspector(wrapper)).toMatchSnapshot();
+
+    nodes = wrapper.find(".node");
+    const protoNode = nodes.at(1);
+    protoNode.simulate("click");
+
+    state = wrapper.state();
+    expect(state.loading.has("root-1/__proto__")).toBeTruthy();
+
+    // Once all the loading promises are resolved, the loading
+    // state property should be cleaned up, and actors and loadedProperties
+    // should have the expected values.
+    await Promise.all(state.loading.get("root-1/__proto__"));
+    expect(formatObjectInspector(wrapper)).toMatchSnapshot();
+    state = wrapper.state();
+
+    expect(state.loading.has("root-1/__proto__")).toBeFalsy();
+    expect(state.loadedProperties.has("root-1/__proto__")).toBeTruthy();
+    expect(state.actors.has(protoStub.prototype.actor)).toBeTruthy();
   });
 
   it("has the expected state when expanding a proxy node", async () => {
@@ -143,7 +174,8 @@ describe("ObjectInspector - state", () => {
     state = wrapper.state();
     expect(state.loading.has("root-2")).toBeFalsy();
     expect(state.loadedProperties.get("root-2")).toEqual(protoStub);
-    expect(state.actors.has(gripRepStubs.get("testProxy").actor)).toBeTruthy();
+    // We don't want to track root actors.
+    expect(state.actors.has(gripRepStubs.get("testProxy").actor)).toBeFalsy();
 
     nodes = wrapper.find(".node");
     const protoNode = nodes.at(4);