From b17d958b07b7c3469c5fee8c66ea3d1b227d9979 Mon Sep 17 00:00:00 2001 From: David Kaltschmidt Date: Thu, 27 Aug 2015 16:55:19 +0200 Subject: [PATCH 1/2] use immutable objects for storing nodes makes checking for topology changes easier, which triggers fewer re-renders fix JS store tests with Immutables only update graph if topology changed --- client/app/scripts/charts/nodes-chart.js | 61 ++++++++++--------- .../stores/__tests__/app-store-test.js | 33 +++++++--- client/app/scripts/stores/app-store.js | 55 +++++++++++------ client/package.json | 1 + 4 files changed, 94 insertions(+), 56 deletions(-) diff --git a/client/app/scripts/charts/nodes-chart.js b/client/app/scripts/charts/nodes-chart.js index 2fe09476c..14e0e8e4c 100644 --- a/client/app/scripts/charts/nodes-chart.js +++ b/client/app/scripts/charts/nodes-chart.js @@ -43,14 +43,13 @@ const NodesChart = React.createClass({ }, componentWillReceiveProps: function(nextProps) { - if (this.getTopologyFingerprint(nextProps.nodes) !== this.getTopologyFingerprint(this.props.nodes)) { + if (nextProps.nodes !== this.props.nodes) { this.setState({ nodes: {}, edges: {} }); + this.updateGraphState(nextProps); } - - this.updateGraphState(nextProps); }, componentWillUnmount: function() { @@ -130,7 +129,7 @@ const NodesChart = React.createClass({ const centerY = this.props.height / 2; const nodes = {}; - _.each(topology, function(node, id) { + topology.forEach(function(node, id) { nodes[id] = prevNodes[id] || {}; // use cached positions if available @@ -141,14 +140,13 @@ const NodesChart = React.createClass({ // copy relevant fields to state nodes _.assign(nodes[id], { - adjacency: node.adjacency, id: id, - label: node.label_major, - pseudo: node.pseudo, - subLabel: node.label_minor, - degree: _.size(node.adjacency) + label: node.get('label_major'), + pseudo: node.get('pseudo'), + subLabel: node.get('label_minor'), + rank: node.get('rank') }); - }, this); + }); return nodes; }, @@ -156,34 +154,37 @@ const NodesChart = React.createClass({ initEdges: function(topology, nodes) { const edges = {}; - _.each(topology, function(node) { - _.each(node.adjacency, function(adjacent) { - const edge = [node.id, adjacent]; - const edgeId = edge.join(Naming.EDGE_ID_SEPARATOR); + topology.forEach(function(node, nodeId) { + const adjacency = node.get('adjacency'); + if (adjacency) { + adjacency.forEach(function(adjacent) { + const edge = [nodeId, adjacent]; + const edgeId = edge.join(Naming.EDGE_ID_SEPARATOR); - if (!edges[edgeId]) { - const source = nodes[edge[0]]; - const target = nodes[edge[1]]; + if (!edges[edgeId]) { + const source = nodes[edge[0]]; + const target = nodes[edge[1]]; - if (!source || !target) { - debug('Missing edge node', edge[0], source, edge[1], target); + if (!source || !target) { + debug('Missing edge node', edge[0], source, edge[1], target); + } + + edges[edgeId] = { + id: edgeId, + value: 1, + source: source, + target: target + }; } - - edges[edgeId] = { - id: edgeId, - value: 1, - source: source, - target: target - }; - } - }); - }, this); + }); + } + }); return edges; }, updateGraphState: function(props) { - const n = _.size(props.nodes); + const n = props.nodes.size; if (n === 0) { return; diff --git a/client/app/scripts/stores/__tests__/app-store-test.js b/client/app/scripts/stores/__tests__/app-store-test.js index de0adf6f0..31abf96e4 100644 --- a/client/app/scripts/stores/__tests__/app-store-test.js +++ b/client/app/scripts/stores/__tests__/app-store-test.js @@ -7,7 +7,24 @@ describe('AppStore', function() { // fixtures - const NODE_SET = {n1: {id: 'n1'}, n2: {id: 'n2'}}; + const NODE_SET = { + n1: { + id: 'n1', + rank: undefined, + adjacency: undefined, + pseudo: undefined, + label_major: undefined, + label_minor: undefined + }, + n2: { + id: 'n2', + rank: undefined, + adjacency: undefined, + pseudo: undefined, + label_major: undefined, + label_minor: undefined + } + }; // actions @@ -113,7 +130,7 @@ describe('AppStore', function() { it('shows nodes that were received', function() { registeredCallback(ReceiveNodesDeltaAction); - expect(AppStore.getNodes()).toEqual(NODE_SET); + expect(AppStore.getNodes().toJS()).toEqual(NODE_SET); }); it('gets selected node after click', function() { @@ -121,11 +138,11 @@ describe('AppStore', function() { registeredCallback(ClickNodeAction); expect(AppStore.getSelectedNodeId()).toBe('n1'); - expect(AppStore.getNodes()).toEqual(NODE_SET); + expect(AppStore.getNodes().toJS()).toEqual(NODE_SET); registeredCallback(HitEscAction) expect(AppStore.getSelectedNodeId()).toBe(null); - expect(AppStore.getNodes()).toEqual(NODE_SET); + expect(AppStore.getNodes().toJS()).toEqual(NODE_SET); }); it('keeps showing nodes on navigating back after node click', function() { @@ -144,7 +161,7 @@ describe('AppStore', function() { RouteAction.state = {"topologyId":"topo1","selectedNodeId": null}; registeredCallback(RouteAction); expect(AppStore.getSelectedNodeId()).toBe(null); - expect(AppStore.getNodes()).toEqual(NODE_SET); + expect(AppStore.getNodes().toJS()).toEqual(NODE_SET); }); it('closes details when changing topologies', function() { @@ -168,17 +185,17 @@ describe('AppStore', function() { it('resets topology on websocket reconnect', function() { registeredCallback(ReceiveNodesDeltaAction); - expect(AppStore.getNodes()).toEqual(NODE_SET); + expect(AppStore.getNodes().toJS()).toEqual(NODE_SET); registeredCallback(CloseWebsocketAction); expect(AppStore.isWebsocketClosed()).toBeTruthy(); - expect(AppStore.getNodes()).toEqual(NODE_SET); + expect(AppStore.getNodes().toJS()).toEqual(NODE_SET); registeredCallback(OpenWebsocketAction); expect(AppStore.isWebsocketClosed()).toBeFalsy(); registeredCallback(ReceiveEmptyNodesDeltaAction); - expect(AppStore.getNodes()).toEqual({}); + expect(AppStore.getNodes().toJS()).toEqual({}); }); diff --git a/client/app/scripts/stores/app-store.js b/client/app/scripts/stores/app-store.js index 08731fed9..385530f21 100644 --- a/client/app/scripts/stores/app-store.js +++ b/client/app/scripts/stores/app-store.js @@ -2,11 +2,14 @@ const EventEmitter = require('events').EventEmitter; const _ = require('lodash'); const assign = require('object-assign'); const debug = require('debug')('scope:app-store'); +const Immutable = require('immutable'); const AppDispatcher = require('../dispatcher/app-dispatcher'); const ActionTypes = require('../constants/action-types'); const Naming = require('../constants/naming'); +const makeOrderedMap = Immutable.OrderedMap; + // Helpers function findCurrentTopology(subTree, topologyId) { @@ -27,6 +30,17 @@ function findCurrentTopology(subTree, topologyId) { return foundTopology; } +function makeNode(node) { + return { + id: node.id, + label_major: node.label_major, + label_minor: node.label_minor, + rank: node.rank, + pseudo: node.pseudo, + adjacency: node.adjacency + }; +} + // Initial values let currentTopologyId = 'containers'; @@ -34,7 +48,7 @@ let errorUrl = null; let version = ''; let mouseOverEdgeId = null; let mouseOverNodeId = null; -let nodes = {}; +let nodes = makeOrderedMap(); let nodeDetails = null; let selectedNodeId = null; let topologies = []; @@ -76,15 +90,17 @@ const AppStore = assign({}, EventEmitter.prototype, { getHighlightedEdgeIds: function() { if (mouseOverNodeId) { // all neighbour combinations because we dont know which direction exists - const node = nodes[mouseOverNodeId]; - return _.flatten( - _.map(node.adjacency, function(nodeId) { - return [ - [nodeId, mouseOverNodeId].join(Naming.EDGE_ID_SEPARATOR), - [mouseOverNodeId, nodeId].join(Naming.EDGE_ID_SEPARATOR) - ]; - }) - ); + const adjacency = nodes.get(mouseOverNodeId).get('adjacency'); + if (adjacency) { + return _.flatten( + adjacency.forEach(function(nodeId) { + return [ + [nodeId, mouseOverNodeId].join(Naming.EDGE_ID_SEPARATOR), + [mouseOverNodeId, nodeId].join(Naming.EDGE_ID_SEPARATOR) + ]; + }) + ); + } } if (mouseOverEdgeId) { return mouseOverEdgeId; @@ -94,8 +110,10 @@ const AppStore = assign({}, EventEmitter.prototype, { getHighlightedNodeIds: function() { if (mouseOverNodeId) { - const node = nodes[mouseOverNodeId]; - return _.union(node.adjacency, [mouseOverNodeId]); + const adjacency = nodes.get(mouseOverNodeId).get('adjacency'); + if (adjacency) { + return _.union(adjacency.toJS(), [mouseOverNodeId]); + } } if (mouseOverEdgeId) { return mouseOverEdgeId.split(Naming.EDGE_ID_SEPARATOR); @@ -152,12 +170,13 @@ AppStore.registeredCallback = function(payload) { selectedNodeId = null; if (payload.topologyId !== currentTopologyId) { currentTopologyId = payload.topologyId; - nodes = {}; + nodes = nodes.clear(); } AppStore.emit(AppStore.CHANGE_EVENT); break; case ActionTypes.CLOSE_WEBSOCKET: + nodes = nodes.clear(); websocketClosed = true; AppStore.emit(AppStore.CHANGE_EVENT); break; @@ -221,20 +240,20 @@ AppStore.registeredCallback = function(payload) { if (mouseOverNodeId === nodeId) { mouseOverNodeId = null; } - if (nodes[nodeId] && _.contains(mouseOverEdgeId, nodeId)) { + if (nodes.has(nodeId) && _.contains(mouseOverEdgeId, nodeId)) { mouseOverEdgeId = null; } - delete nodes[nodeId]; + nodes = nodes.delete(nodeId); }); // update existing nodes _.each(payload.delta.update, function(node) { - nodes[node.id] = node; + nodes = nodes.set(node.id, nodes.get(node.id).mergeDeep(makeNode(node))); }); // add new nodes _.each(payload.delta.add, function(node) { - nodes[node.id] = node; + nodes = nodes.set(node.id, Immutable.fromJS(makeNode(node))); }); AppStore.emit(AppStore.CHANGE_EVENT); @@ -254,7 +273,7 @@ AppStore.registeredCallback = function(payload) { case ActionTypes.ROUTE_TOPOLOGY: if (currentTopologyId !== payload.state.topologyId) { - nodes = {}; + nodes = nodes.clear(); } currentTopologyId = payload.state.topologyId; selectedNodeId = payload.state.selectedNodeId; diff --git a/client/package.json b/client/package.json index f9268e64e..a2f4fd6a6 100644 --- a/client/package.json +++ b/client/package.json @@ -12,6 +12,7 @@ "flux": "^2.0.3", "font-awesome": "^4.3.0", "font-awesome-webpack": "0.0.3", + "immutable": "^3.7.4", "keymirror": "^0.1.1", "lodash": "~3.9.3", "material-ui": "~0.7.5", From 2e50f6ae76b5dc5748cdb1cc3689549f479ff051 Mon Sep 17 00:00:00 2001 From: David Kaltschmidt Date: Fri, 28 Aug 2015 12:54:39 +0200 Subject: [PATCH 2/2] add test to check for detail pane titles --- .../components/__tests__/node-details-test.js | 31 +++++++++++++++++++ client/app/scripts/components/node-details.js | 4 +-- client/test/karma.conf.js | 1 + client/test/polyfill.js | 21 +++++++++++++ 4 files changed, 55 insertions(+), 2 deletions(-) create mode 100644 client/app/scripts/components/__tests__/node-details-test.js create mode 100644 client/test/polyfill.js diff --git a/client/app/scripts/components/__tests__/node-details-test.js b/client/app/scripts/components/__tests__/node-details-test.js new file mode 100644 index 000000000..7fc1d2dc1 --- /dev/null +++ b/client/app/scripts/components/__tests__/node-details-test.js @@ -0,0 +1,31 @@ +describe('NodeDetails', () => { + let NodeDetails; + let nodes; + let nodeId; + let details; + const React = require('react'); + const Immutable = require('immutable'); + const TestUtils = require('react/lib/ReactTestUtils'); + + beforeEach(() => { + NodeDetails = require('../node-details.js'); + nodes = Immutable.OrderedMap(); + nodeId = 'n1'; + }); + + it('shows n/a when node was not found', () => { + const c = TestUtils.renderIntoDocument(); + const notFound = TestUtils.findRenderedDOMComponentWithClass(c, 'node-details-header-notavailable'); + expect(notFound).toBeDefined(); + }); + + it('show label of node with title', () => { + nodes = nodes.set(nodeId, Immutable.fromJS({id: nodeId})); + details = {label_major: 'Node 1', tables: []}; + const c = TestUtils.renderIntoDocument(); + + const title = TestUtils.findRenderedDOMComponentWithClass(c, 'node-details-header-label'); + expect(title.getDOMNode().textContent).toBe('Node 1'); + }); + +}); \ No newline at end of file diff --git a/client/app/scripts/components/node-details.js b/client/app/scripts/components/node-details.js index 1c74c0db9..0696bd79e 100644 --- a/client/app/scripts/components/node-details.js +++ b/client/app/scripts/components/node-details.js @@ -47,7 +47,7 @@ const NodeDetails = React.createClass({ render: function() { const details = this.props.details; - const nodeExists = this.props.nodes[this.props.nodeId]; + const nodeExists = this.props.nodes && this.props.nodes.has(this.props.nodeId); if (!nodeExists) { return this.renderNotAvailable(); @@ -58,7 +58,7 @@ const NodeDetails = React.createClass({ } const style = { - 'background-color': this.getNodeColorDark(details.label_major) + 'backgroundColor': this.getNodeColorDark(details.label_major) }; return ( diff --git a/client/test/karma.conf.js b/client/test/karma.conf.js index 1d1a39905..fd7979048 100644 --- a/client/test/karma.conf.js +++ b/client/test/karma.conf.js @@ -4,6 +4,7 @@ module.exports = function(config) { 'PhantomJS' ], files: [ + './polyfill.js', { pattern: 'tests.webpack.js', watched: false diff --git a/client/test/polyfill.js b/client/test/polyfill.js new file mode 100644 index 000000000..40e036c7d --- /dev/null +++ b/client/test/polyfill.js @@ -0,0 +1,21 @@ +/** + * Function.prototype.bind polyfill used by PhantomJS + */ +if (typeof Function.prototype.bind != 'function') { + Function.prototype.bind = function bind(obj) { + var args = Array.prototype.slice.call(arguments, 1), + self = this, + nop = function() { + }, + bound = function() { + return self.apply( + this instanceof nop ? this : (obj || {}), args.concat( + Array.prototype.slice.call(arguments) + ) + ); + }; + nop.prototype = this.prototype || {}; + bound.prototype = new nop(); + return bound; + }; +}