Ensuring long strings get re-rendered when their loaded properties are updated (firefox-devtools#7874) (firefox-devtools#8053)

Author: yzen

packages/devtools-components/src/tests/__snapshots__/tree.js.snap

@@ -266,6 +266,20 @@ exports[`Tree active item - renders as expected when using keyboard and Space 1`
 "
 `;
 
+exports[`Tree calls shouldItemUpdate when provided 1`] = `
+"
+▶︎ A
+▶︎ M
+"
+`;
+
+exports[`Tree calls shouldItemUpdate when provided 2`] = `
+"
+▶︎ A
+▶︎ M
+"
+`;
+
 exports[`Tree ignores key strokes when pressing modifiers 1`] = `
 "
 ▼ A

packages/devtools-components/src/tests/tree.js

@@ -203,6 +203,30 @@ describe("Tree", () => {
     expect(formatTree(wrapper)).toMatchSnapshot();
   });
 
+  it("calls shouldItemUpdate when provided", () => {
+    const shouldItemUpdate = jest.fn((prev, next) => true);
+    const wrapper = mountTree({
+      shouldItemUpdate
+    });
+    expect(formatTree(wrapper)).toMatchSnapshot();
+    expect(shouldItemUpdate.mock.calls).toHaveLength(0);
+
+    wrapper
+      .find("Tree")
+      .first()
+      .instance()
+      .forceUpdate();
+    expect(formatTree(wrapper)).toMatchSnapshot();
+    expect(shouldItemUpdate.mock.calls).toHaveLength(2);
+
+    expect(shouldItemUpdate.mock.calls[0][0]).toBe("A");
+    expect(shouldItemUpdate.mock.calls[0][1]).toBe("A");
+    expect(shouldItemUpdate.mock.results[0].value).toBe(true);
+    expect(shouldItemUpdate.mock.calls[1][0]).toBe("M");
+    expect(shouldItemUpdate.mock.calls[1][1]).toBe("M");
+    expect(shouldItemUpdate.mock.results[1].value).toBe(true);
+  });
+
   it("active item - renders as expected when clicking away", () => {
     const wrapper = mountTree({
       expanded: new Set("ABCDEFGHIJKLMNO".split("")),

packages/devtools-components/src/tree.js

@@ -66,6 +66,7 @@ class TreeNode extends Component {
       item: PropTypes.any.isRequired,
       isExpandable: PropTypes.bool.isRequired,
       onClick: PropTypes.func,
+      shouldItemUpdate: PropTypes.func,
       renderItem: PropTypes.func.isRequired
     };
   }
@@ -96,6 +97,8 @@ class TreeNode extends Component {
   shouldComponentUpdate(nextProps) {
     return (
       this.props.item !== nextProps.item ||
+      (this.props.shouldItemUpdate &&
+        this.props.shouldItemUpdate(this.props.item, nextProps.item)) ||
       this.props.focused !== nextProps.focused ||
       this.props.expanded !== nextProps.expanded
     );
@@ -338,6 +341,17 @@ class Tree extends Component {
       //     getChildren: item => item.children
       getChildren: PropTypes.func.isRequired,
 
+      // A function to check if the tree node for the item should be updated.
+      //
+      // Type: shouldItemUpdate(prevItem: Item, nextItem: Item) -> Boolean
+      //
+      // Example:
+      //
+      //     // This item should be updated if it's type is a long string
+      //     shouldItemUpdate: (prevItem, nextItem) =>
+      //       nextItem.type === "longstring"
+      shouldItemUpdate: PropTypes.func,
+
       // A function which takes an item and ArrowExpander component instance and
       // returns a component, or text, or anything else that React considers
       // renderable.
@@ -929,6 +943,7 @@ class Tree extends Component {
         index: i,
         item,
         depth,
+        shouldItemUpdate: this.props.shouldItemUpdate,
         renderItem: this.props.renderItem,
         focused: focused === item,
         active: active === item,

packages/devtools-reps/src/object-inspector/components/ObjectInspector.js

@@ -24,6 +24,7 @@ const {
   getChildrenWithEvaluations,
   getActor,
   getParent,
+  getValue,
   nodeIsPrimitive,
   nodeHasGetter,
   nodeHasSetter
@@ -78,6 +79,7 @@ class ObjectInspector extends Component<Props> {
     self.activateItem = this.activateItem.bind(this);
     self.getRoots = this.getRoots.bind(this);
     self.getNodeKey = this.getNodeKey.bind(this);
+    self.shouldItemUpdate = this.shouldItemUpdate.bind(this);
   }
 
   componentWillMount() {
@@ -236,6 +238,13 @@ class ObjectInspector extends Component<Props> {
     }
   }
 
+  shouldItemUpdate(prevItem: Node, nextItem: Node) {
+    const value = getValue(nextItem);
+    // Long string should always update because fullText loading will not
+    // trigger item re-render.
+    return value && value.type === "longString";
+  }
+
   render() {
     const {
       autoExpandAll = true,
@@ -271,6 +280,7 @@ class ObjectInspector extends Component<Props> {
       onFocus: focusable ? this.focusItem : null,
       onActivate: focusable ? this.activateItem : null,
 
+      shouldItemUpdate: this.shouldItemUpdate,
       renderItem: (item, depth, focused, arrow, expanded) =>
         ObjectInspectorItem({
           ...this.props,

packages/devtools-reps/src/object-inspector/tests/component/should-item-update.js

@@ -0,0 +1,92 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at <http://mozilla.org/MPL/2.0/>. */
+
+/* global jest */
+
+const { mountObjectInspector } = require("../test-utils");
+const ObjectClient = require("../__mocks__/object-client");
+const LongStringClient = require("../__mocks__/long-string-client");
+
+const repsPath = "../../../reps";
+const longStringStubs = require(`${repsPath}/stubs/long-string`);
+const gripStubs = require(`${repsPath}/stubs/grip`);
+
+function mount(stub) {
+  const root = {
+    path: "root",
+    contents: {
+      value: stub
+    }
+  };
+
+  const { wrapper } = mountObjectInspector({
+    client: {
+      createObjectClient: grip => ObjectClient(grip),
+      createLongStringClient: grip => LongStringClient(grip)
+    },
+    props: {
+      roots: [root]
+    }
+  });
+
+  return { wrapper, root };
+}
+
+describe("shouldItemUpdate", () => {
+  it("for longStrings", () => {
+    shouldItemUpdateCheck(longStringStubs.get("testUnloadedFullText"), true, 2);
+  });
+
+  it("for basic object", () => {
+    shouldItemUpdateCheck(gripStubs.get("testBasic"), false, 1);
+  });
+});
+
+function shouldItemUpdateCheck(
+  stub,
+  shouldItemUpdateResult,
+  renderCallsLength
+) {
+  const { root, wrapper } = mount(stub);
+
+  const shouldItemUpdateSpy = getShouldItemUpdateSpy(wrapper);
+  const treeNodeRenderSpy = getTreeNodeRenderSpy(wrapper);
+
+  updateObjectInspectorTree(wrapper);
+
+  checkShouldItemUpdate(shouldItemUpdateSpy, root, shouldItemUpdateResult);
+  expect(treeNodeRenderSpy.mock.calls).toHaveLength(renderCallsLength);
+}
+
+function checkShouldItemUpdate(spy, item, result) {
+  expect(spy.mock.calls).toHaveLength(1);
+  expect(spy.mock.calls[0][0]).toBe(item);
+  expect(spy.mock.calls[0][1]).toBe(item);
+  expect(spy.mock.results[0].value).toBe(result);
+}
+
+function getInstance(wrapper, selector) {
+  return wrapper
+    .find(selector)
+    .first()
+    .instance();
+}
+
+function getShouldItemUpdateSpy(wrapper) {
+  return jest.spyOn(
+    getInstance(wrapper, "ObjectInspector"),
+    "shouldItemUpdate"
+  );
+}
+
+function getTreeNodeRenderSpy(wrapper) {
+  return jest.spyOn(getInstance(wrapper, "TreeNode"), "render");
+}
+
+function updateObjectInspectorTree(wrapper) {
+  // Update the ObjectInspector first to propagate its updated options to the
+  // Tree component.
+  getInstance(wrapper, "ObjectInspector").forceUpdate();
+  getInstance(wrapper, "Tree").forceUpdate();
+}