Fix/graph unnecessary calls to graph forces config (#114)

danielcaldas · web-flow · commit 52df1b468641 · 2018-09-30T00:19:18.000+02:00
* Add deprecation note on componentWillReceiveProps

* Add antiPick method to utils

* Initialize links in initializeGraphState

* Add check:light task in package.json

* Implement checkForGraphElementsChanges in graph.helper

* e2e test collapse improvements

* Small check on number of links graph e2e

* Proper link initialization by mapping input to d3 link
diff --git a/cypress/integration/graph.e2e.js b/cypress/integration/graph.e2e.js
@@ -65,6 +65,7 @@ describe('[rd3g-graph] graph tests', function() {
                 it('nodes props modifications should be reflected in the graph', function() {
                     cy.get('text').should('have.length', 14);
                     cy.get('path[class="link"]').should('be.visible');
+                    cy.get('path[class="link"]').should('have.length', 23);
 
                     this.sandboxPO.addNode();
                     this.sandboxPO.addNode();
@@ -73,6 +74,9 @@ describe('[rd3g-graph] graph tests', function() {
 
                     cy.get('text').should('have.length', 18);
 
+                    // should now have more than 23 links
+                    cy.get('path[class="link"]').should('not.have.length', 23);
+
                     // click (+) add prop to 1st node
                     this.sandboxPO.addJsonTreeFirstNodeProp();
                     // prop name be color
@@ -125,6 +129,8 @@ describe('[rd3g-graph] graph tests', function() {
 
             this.node1PO = new NodePO(1);
             this.node2PO = new NodePO(2);
+            this.node3PO = new NodePO(3);
+            this.node4PO = new NodePO(4);
             this.link12PO = new LinkPO(0);
         });
 
@@ -141,6 +147,19 @@ describe('[rd3g-graph] graph tests', function() {
             // Check the leaf node & link is no longer visible
             this.node2PO.getPath().should('not.be.visible');
             line.should('not.be.visible');
+
+            // Check if other nodes and links are still visible
+            this.node1PO.getPath().should('be.visible');
+            this.node3PO.getPath().should('be.visible');
+            this.node4PO.getPath().should('be.visible');
+
+            const link13PO = new LinkPO(1);
+            const link14PO = new LinkPO(2);
+            const link34PO = new LinkPO(3);
+
+            link13PO.getLine().should('be.visible');
+            link14PO.getLine().should('be.visible');
+            link34PO.getLine().should('be.visible');
         });
     });
 });
diff --git a/cypress/integration/link.e2e.js b/cypress/integration/link.e2e.js
@@ -6,7 +6,7 @@ const NodePO = require('../page-objects/node.po');
 const SandboxPO = require('../page-objects/sandbox.po');
 let nodes = require('./../../sandbox/data/small/small.data').nodes.map(({ id }) => id);
 
-describe('[rd3g-graph] link tests', function() {
+describe('[rd3g-link] link tests', function() {
     before(function() {
         this.sandboxPO = new SandboxPO();
         // visit sandbox
diff --git a/package.json b/package.json
@@ -6,6 +6,7 @@
     "license": "MIT",
     "scripts": {
         "check": "npm run docs:lint && npm run lint && npm run test && npm run functional",
+        "check:light": "npm run lint && npm run test",
         "dev": "NODE_ENV=dev webpack-dev-server --mode=development --content-base sandbox --config webpack.config.js --inline --hot --port 3002",
         "dist:rd3g": "rm -rf dist/ && webpack --config webpack.config.dist.js -p --display-modules --optimize-minimize",
         "dist:sandbox": "webpack --config webpack.config.js -p",
diff --git a/src/components/graph/Graph.jsx b/src/components/graph/Graph.jsx
@@ -295,16 +295,22 @@ export default class Graph extends React.Component {
         this.state = graphHelper.initializeGraphState(this.props, this.state);
     }
 
+    /**
+     * @deprecated
+     * `componentWillReceiveProps` has a replacement method in react v16.3 onwards.
+     * that is getDerivedStateFromProps.
+     * But one needs to be aware that if an anti pattern of `componentWillReceiveProps` is
+     * in place for this implementation the migration might not be that easy.
+     * See {@link https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html}.
+     * @param {Object} nextProps - props.
+     * @returns {undefined}
+     */
     componentWillReceiveProps(nextProps) {
-        const newGraphElements =
-            nextProps.data.nodes.length !== this.state.nodesInputSnapshot.length ||
-            nextProps.data.links.length !== this.state.linksInputSnapshot.length ||
-            !utils.isDeepEqual(nextProps.data, {
-                nodes: this.state.nodesInputSnapshot,
-                links: this.state.linksInputSnapshot
-            });
-        const state = newGraphElements ? graphHelper.initializeGraphState(nextProps, this.state) : this.state;
-
+        const { graphElementsUpdated, newGraphElements } = graphHelper.checkForGraphElementsChanges(
+            nextProps,
+            this.state
+        );
+        const state = graphElementsUpdated ? graphHelper.initializeGraphState(nextProps, this.state) : this.state;
         const newConfig = nextProps.config || {};
         const configUpdated =
             newConfig && !utils.isObjectEmpty(newConfig) && !utils.isDeepEqual(newConfig, this.state.config);
@@ -318,8 +324,8 @@ export default class Graph extends React.Component {
         this.setState({
             ...state,
             config,
-            newGraphElements,
             configUpdated,
+            newGraphElements,
             transform
         });
     }
diff --git a/src/components/graph/graph.helper.js b/src/components/graph/graph.helper.js
@@ -1,3 +1,4 @@
+/*eslint-disable max-lines*/
 /**
  * @module Graph/helper
  * @description
@@ -148,6 +149,39 @@ function _initializeNodes(graphNodes) {
     return nodes;
 }
 
+/**
+ * Maps an input link (with format `{ source: 'sourceId', target: 'targetId' }`) to a d3Link
+ * (with format `{ source: { id: 'sourceId' }, target: { id: 'targetId' } }`). If d3Link with
+ * given index exists already that same d3Link is returned.
+ * @param {Object} link - input link.
+ * @param {number} index - index of the input link.
+ * @param {Array.<Object>} d3Links - all d3Links.
+ * @returns {Object} a d3Link.
+ */
+function _mapDataLinkToD3Link(link, index, d3Links = []) {
+    const d3Link = d3Links[index];
+
+    if (d3Link) {
+        return d3Link;
+    }
+
+    const highlighted = false;
+    const source = {
+        id: link.source,
+        highlighted
+    };
+    const target = {
+        id: link.target,
+        highlighted
+    };
+
+    return {
+        index,
+        source,
+        target
+    };
+}
+
 /**
  * Some integrity validations on links and nodes structure. If some validation fails the function will
  * throw an error.
@@ -322,6 +356,42 @@ function buildNodeProps(node, config, nodeCallbacks = {}, highlightedNode, highl
     };
 }
 
+// list of properties that are of no interest when it comes to nodes and links comparison
+const NODE_PROPERTIES_DISCARD_TO_COMPARE = ['x', 'y', 'vx', 'vy', 'index'];
+
+/**
+ * This function checks for graph elements (nodes and links) changes, in two different
+ * levels of significance, updated elements (whether some property has changed in some
+ * node or link) and new elements (whether some new elements or added/removed from the graph).
+ * @param {Object} nextProps - nextProps that graph will receive.
+ * @param {Object} currentState - the current state of the graph.
+ * @returns {Object.<string, boolean>} returns object containing update check flags:
+ * - newGraphElements - flag that indicates whether new graph elements were added.
+ * - graphElementsUpdated - flag that indicates whether some graph elements have
+ * updated (some property that is not in NODE_PROPERTIES_DISCARD_TO_COMPARE was added to
+ * some node or link or was updated).
+ */
+function checkForGraphElementsChanges(nextProps, currentState) {
+    const nextNodes = nextProps.data.nodes.map(n => utils.antiPick(n, NODE_PROPERTIES_DISCARD_TO_COMPARE));
+    const nextLinks = nextProps.data.links;
+    const stateD3Nodes = currentState.d3Nodes.map(n => utils.antiPick(n, NODE_PROPERTIES_DISCARD_TO_COMPARE));
+    const stateD3Links = currentState.d3Links.map(l => ({
+        // FIXME: solve this source data inconsistency later
+        source: l.source.id || l.source,
+        target: l.target.id || l.target
+    }));
+    const graphElementsUpdated = !(
+        utils.isDeepEqual(nextNodes, stateD3Nodes) && utils.isDeepEqual(nextLinks, stateD3Links)
+    );
+    const newGraphElements =
+        nextNodes.length !== stateD3Nodes.length ||
+        nextLinks.length !== stateD3Links.length ||
+        !utils.isDeepEqual(nextNodes.map(({ id }) => ({ id })), stateD3Nodes.map(({ id }) => ({ id }))) ||
+        !utils.isDeepEqual(nextLinks, stateD3Links.map(({ source, target }) => ({ source, target })));
+
+    return { graphElementsUpdated, newGraphElements };
+}
+
 /**
  * Encapsulates common procedures to initialize graph.
  * @param {Object} props - Graph component props, object that holds data, id and config.
@@ -333,33 +403,29 @@ function buildNodeProps(node, config, nodeCallbacks = {}, highlightedNode, highl
  * @memberof Graph/helper
  */
 function initializeGraphState({ data, id, config }, state) {
-    let graph;
-
     _validateGraphData(data);
 
+    let graph;
     const nodesInputSnapshot = data.nodes.map(n => Object.assign({}, n));
     const linksInputSnapshot = data.links.map(l => Object.assign({}, l));
 
-    if (state && state.nodes && state.links) {
-        // absorb existent positioning
+    if (state && state.nodes) {
         graph = {
             nodes: data.nodes.map(
                 n =>
                     state.nodes[n.id]
                         ? Object.assign({}, n, utils.pick(state.nodes[n.id], NODE_PROPS_WHITELIST))
                         : Object.assign({}, n)
             ),
-            links: {}
+            links: data.links.map((l, index) => _mapDataLinkToD3Link(l, index, state && state.d3Links))
         };
     } else {
         graph = {
             nodes: data.nodes.map(n => Object.assign({}, n)),
-            links: {}
+            links: data.links.map(l => Object.assign({}, l))
         };
     }
 
-    graph.links = data.links.map(l => Object.assign({}, l));
-
     let newConfig = Object.assign({}, utils.merge(DEFAULT_CONFIG, config || {}));
     let nodes = _initializeNodes(graph.nodes);
     let links = _initializeLinks(graph.links); // matrix of graph connections
@@ -513,8 +579,10 @@ function getNodeCardinality(nodeId, linksMatrix) {
 }
 
 export {
+    NODE_PROPS_WHITELIST,
     buildLinkProps,
     buildNodeProps,
+    checkForGraphElementsChanges,
     disconnectLeafNodeConnections,
     getLeafNodeConnections,
     getNodeCardinality,
diff --git a/src/utils.js b/src/utils.js
@@ -127,6 +127,18 @@ function pick(o, props = []) {
     }, {});
 }
 
+/**
+ * Picks all props except the ones passed in the props array.
+ * @param {Object} o - the object to pick props from.
+ * @param {Array.<string>} props - list of props that we DON'T want to pick from o.
+ * @returns {Object} the object resultant from the anti picking operation.
+ */
+function antiPick(o, props = []) {
+    const wanted = Object.keys(o).filter(k => !props.includes(k));
+
+    return pick(o, wanted);
+}
+
 /**
  * Helper function for customized error logging.
  * @param  {string} component - the name of the component where the error is to be thrown.
@@ -145,5 +157,6 @@ export default {
     isObjectEmpty,
     merge,
     pick,
+    antiPick,
     throwErr
 };
diff --git a/test/component/graph/graph.helper.test.js b/test/component/graph/graph.helper.test.js
@@ -235,7 +235,7 @@ describe('Graph Helper', () => {
             });
 
             describe('and received state was already initialized', () => {
-                test('should create graph structure absorbing stored nodes behavior in state obj', () => {
+                test('should create graph structure absorbing stored nodes and links behavior', () => {
                     const data = {
                         nodes: [{ id: 'A' }, { id: 'B' }, { id: 'C' }],
                         links: [{ source: 'A', target: 'B' }, { source: 'C', target: 'A' }]
@@ -245,7 +245,7 @@ describe('Graph Helper', () => {
                             A: { x: 20, y: 40 },
                             B: { x: 40, y: 60 }
                         },
-                        links: 'links',
+                        links: [],
                         nodeIndexMapping: 'nodeIndexMapping'
                     };
 
@@ -273,12 +273,26 @@ describe('Graph Helper', () => {
                     ]);
                     expect(newState.d3Links).toEqual([
                         {
-                            source: 'A',
-                            target: 'B'
+                            index: 0,
+                            source: {
+                                highlighted: false,
+                                id: 'A'
+                            },
+                            target: {
+                                highlighted: false,
+                                id: 'B'
+                            }
                         },
                         {
-                            source: 'C',
-                            target: 'A'
+                            index: 1,
+                            source: {
+                                highlighted: false,
+                                id: 'C'
+                            },
+                            target: {
+                                highlighted: false,
+                                id: 'A'
+                            }
                         }
                     ]);
                 });
@@ -364,12 +378,26 @@ describe('Graph Helper', () => {
                     configUpdated: false,
                     d3Links: [
                         {
-                            source: 'A',
-                            target: 'B'
+                            index: 0,
+                            source: {
+                                highlighted: false,
+                                id: 'A'
+                            },
+                            target: {
+                                highlighted: false,
+                                id: 'B'
+                            }
                         },
                         {
-                            source: 'C',
-                            target: 'A'
+                            index: 1,
+                            source: {
+                                highlighted: false,
+                                id: 'C'
+                            },
+                            target: {
+                                highlighted: false,
+                                id: 'A'
+                            }
                         }
                     ],
                     d3Nodes: [
@@ -406,6 +434,16 @@ describe('Graph Helper', () => {
                             A: 1
                         }
                     },
+                    linksInputSnapshot: [
+                        {
+                            source: 'A',
+                            target: 'B'
+                        },
+                        {
+                            source: 'C',
+                            target: 'A'
+                        }
+                    ],
                     newGraphElements: false,
                     nodes: {
                         A: {
@@ -427,10 +465,6 @@ describe('Graph Helper', () => {
                             y: 0
                         }
                     },
-                    simulation: {
-                        force: forceStub
-                    },
-                    transform: 1,
                     nodesInputSnapshot: [
                         {
                             id: 'A'
@@ -442,16 +476,10 @@ describe('Graph Helper', () => {
                             id: 'C'
                         }
                     ],
-                    linksInputSnapshot: [
-                        {
-                            source: 'A',
-                            target: 'B'
-                        },
-                        {
-                            source: 'C',
-                            target: 'A'
-                        }
-                    ]
+                    simulation: {
+                        force: forceStub
+                    },
+                    transform: 1
                 });
             });
         });
diff --git a/test/utils.test.js b/test/utils.test.js
