Use symbols for nodes path.

Fixes #1001, #1002 & #1003.

The path property on a node is very important as it serve
as an identifier to know if a node is expanded, and what
its properties are. Currently, this path was a simple string
which could led to 2 nodes having the same path, which could
have unwanted effect on user interaction.
To avoid this, the path is now a symbol, which has the advantage
of being unique.
Because of this change, we needed to adapt our code so we always
have the same path reference in the state.
With this patch, we'll always dispatch the nodePropertiesLoaded action,
even if the node don't have any properties. We'll store null in the state,
so we can then check if a node was properly handled.
As a result, we change the shouldComponentUpdate of the component
to reflect this new behavior.
In order to have always the same reference, we always add created node
to the cacheNodes property, which we can do because of the change in
loadedProperties.
This should also give us some performance improvements as we don't have
to recreate some nodes twice.
We take this as an opportunity to change the signature of the createNode
function to be more explicit (it now expect an object of options), and
we also create the path of the node from its parent when the path option
is not supplied.

Author: nchevobbe

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

@@ -39,7 +39,10 @@ function nodeExpand(
       type: "NODE_EXPAND",
       data: {node}
     });
-    dispatch(nodeLoadProperties(node, actor, loadedProperties, createObjectClient));
+
+    if (!loadedProperties.has(node.path)) {
+      dispatch(nodeLoadProperties(node, actor, loadedProperties, createObjectClient));
+    }
   };
 }
 
@@ -70,9 +73,7 @@ function nodeLoadProperties(
     try {
       const properties =
         await loadItemProperties(item, createObjectClient, loadedProperties);
-      if (Object.keys(properties).length > 0) {
-        dispatch(nodePropertiesLoaded(item, actor, properties));
-      }
+      dispatch(nodePropertiesLoaded(item, actor, properties));
     } catch (e) {
       console.error(e);
     }

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

@@ -113,9 +113,19 @@ class ObjectInspector extends Component {
       return true;
     }
 
-    return expandedPaths.size !== nextProps.expandedPaths.size
-      || loadedProperties.size !== nextProps.loadedProperties.size
-      || [...expandedPaths].some(key => !nextProps.expandedPaths.has(key));
+    // We should update if:
+    // - there are new loaded loaded properties
+    // - OR the expanded paths number changed, and all of them have properties loaded
+    // - OR the expanded paths number did not changed, but old and new sets differ
+    return loadedProperties.size !== nextProps.loadedProperties.size
+      || (
+        expandedPaths.size !== nextProps.expandedPaths.size &&
+        [...nextProps.expandedPaths].every(path => nextProps.loadedProperties.has(path))
+      )
+      || (
+        expandedPaths.size === nextProps.expandedPaths.size &&
+        [...nextProps.expandedPaths].some(key => !expandedPaths.has(key))
+      );
   }
 
   componentWillUnmount() {
@@ -151,7 +161,9 @@ class ObjectInspector extends Component {
   }
 
   getNodeKey(item: Node) : string {
-    return item.path || JSON.stringify(item);
+    return item.path && typeof item.path.toString === "function"
+      ? item.path.toString()
+      : JSON.stringify(item);
   }
 
   setExpanded(item: Node, expand: boolean) {
@@ -426,7 +438,7 @@ class ObjectInspector extends Component {
       autoExpandDepth,
       disabledFocus,
 
-      isExpanded: item => expandedPaths && expandedPaths.has(this.getNodeKey(item)),
+      isExpanded: item => expandedPaths && expandedPaths.has(item.path),
       isExpandable: item => nodeIsPrimitive(item) === false,
       focused: focusedItem,