Remove oldgraph from Kedro

With recent changes in kedro-viz, the oldgraph is now broken. This PR is to remove the oldgraph (graphDagre) and the flags associated to enable it.

ARCHITECTURE.md

@@ -127,8 +127,3 @@ Kedro-Viz includes a graph layout engine, for details see the [layout engine doc
 Our layout engine runs inside a web worker, which asynchronously performs these expensive calculations in a separate CPU thread, in order to avoid this blocking other operations on the main thread (e.g. CSS transitions and other state updates).
 
 The app uses [redux-watch](https://github.com/ExodusMovement/redux-watch) with a graph input selector to watch the store for state changes relevant to the graph layout. If the layout needs to change, this listener dispatches an asynchronous action which sends a message to the web worker to instruct it to calculate the new layout. Once the layout worker completes its calculations, it returns a new action to update the store's `state.graph` property with the new layout. Updates to the graph input state during worker calculations will interrupt the worker and cause it to start over from scratch.
-
-The logic for the layout calculations are handled in `/src/utils/graph/`. There are two graph layout engines, which can be toggled with the `oldgraph` flag:
-
-1. `dagre`: The previous iteration, which uses [Dagre.js](https://github.com/dagrejs/dagre)
-2. `newgraph`: Our custom built-in layout engine.

README.md

@@ -105,18 +105,15 @@ As a JavaScript React component, the project is designed to be used in two diffe
 
 The following flags are available to toggle experimental features:
 
-- `oldgraph` - From release v3.8.0. Display old version of graph (dagre algorithm) without improved graphing algorithm. (default `false`)
 - `newparams` - From release v3.12.0. Disable parameters on page load and highlight parameter connections.
 - `sizewarning` - From release v3.9.1. Show a warning before rendering very large graphs. (default `true`)
 - `modularpipeline` - From release v3.11.0. Enables filtering of nodes by modular pipelines. Note that selecting both modular pipeline and tag filters will only return nodes that belongs to both categories. (default `false`). 
 
-Note that newgraph has been removed from v3.8.0 onwards and is now the default functionality. Should there be issues with your project, see the oldgraph flag above.
-
 ### Setting flags
 
 To enable or disable a flagged feature, add the flag as a parameter with the value `true` or `false` to the end of the URL in your browser when running Kedro-Viz, e.g.
 
-`http://localhost:4141/?data=demo&oldgraph=true`
+`http://localhost:4141/?data=demo&newparams=true`
 
 The setting you provide persists for all sessions on your machine, until you change it.
 

package.json

@@ -51,7 +51,6 @@
     "d3-shape": "^2.1.0",
     "d3-transition": "^2.0.0",
     "d3-zoom": "^2.0.0",
-    "dagre": "git+https://github.com/richardwestenra/dagre.git#manual-ranking",
     "deepmerge": "^4.2.2",
     "fetch-mock": "^9.11.0",
     "highlight.js": "^10.7.3",

src/actions/graph.js

@@ -28,16 +28,15 @@ export function updateGraph(graph) {
 }
 
 /**
- * Choose which layout engine to use based on the oldgraph flag
+ * Assign layout engine to use based on the newgraph
  * @param {Object} instance Worker parent instance
  * @param {Object} state A subset of main state
  * @return {function} Promise function
  */
-const chooseLayout = (instance, state) =>
-  state.oldgraph ? instance.graphDagre(state) : instance.graphNew(state);
+const layout = (instance, state) => instance.graphNew(state);
 
 // Prepare new layout worker
-const layoutWorker = preventWorkerQueues(worker, chooseLayout);
+const layoutWorker = preventWorkerQueues(worker, layout);
 
 /**
  * Async action to calculate graph layout in a web worker

src/actions/graph.test.js

@@ -37,29 +37,12 @@ describe('graph actions', () => {
       return calculateGraph(getGraphInput(state))(store.dispatch).then(() => {
         expect(store.getState().graph).toEqual(
           expect.objectContaining({
-            oldgraph: expect.any(Boolean),
             nodes: expect.any(Array),
             edges: expect.any(Array),
             size: expect.any(Object),
           })
         );
       });
     });
-
-    it('uses new graph by default if the oldgraph flag is not set', () => {
-      const state = reducer(mockState.animals, changeFlag('oldgraph', false));
-      const store = createStore(reducer, state);
-      return calculateGraph(getGraphInput(state))(store.dispatch).then(() => {
-        expect(store.getState().graph.oldgraph).toBe(false);
-      });
-    });
-
-    it('uses dagre if the oldgraph flag is set to true', () => {
-      const state = reducer(mockState.animals, changeFlag('oldgraph', true));
-      const store = createStore(reducer, state);
-      return calculateGraph(getGraphInput(state))(store.dispatch).then(() => {
-        expect(store.getState().graph.oldgraph).toBe(true);
-      });
-    });
   });
 });

src/config.js

@@ -26,12 +26,6 @@ export const largeGraphThreshold = 1000;
 
 // Remember to update the 'Flags' section in the README when updating these:
 export const flags = {
-  oldgraph: {
-    description: 'Use older Dagre graphing algorithm',
-    default: false,
-    private: false,
-    icon: '📈',
-  },
   newparams: {
     description: `Disable parameters on page load and highlight parameter connections.`,
     default: true,

src/selectors/layout.js

@@ -11,7 +11,6 @@ import {
   codeSidebarWidth,
 } from '../config';
 
-const getOldgraphFlag = (state) => state.flags.oldgraph;
 const getSizeWarningFlag = (state) => state.flags.sizewarning;
 const getVisibleSidebar = (state) => state.visible.sidebar;
 const getVisibleCode = (state) => state.visible.code;
@@ -50,16 +49,15 @@ export const getGraphInput = createSelector(
     getVisibleNodes,
     getVisibleEdges,
     getVisibleLayerIDs,
-    getOldgraphFlag,
     getFontLoaded,
     getTriggerLargeGraphWarning,
   ],
-  (nodes, edges, layers, oldgraph, fontLoaded, triggerLargeGraphWarning) => {
+  (nodes, edges, layers, fontLoaded, triggerLargeGraphWarning) => {
     if (!fontLoaded || triggerLargeGraphWarning) {
       return null;
     }
 
-    return { nodes, edges, layers, oldgraph, fontLoaded };
+    return { nodes, edges, layers, fontLoaded };
   }
 );
 

src/selectors/layout.test.js

@@ -13,7 +13,7 @@ import {
 import { updateGraph } from '../actions/graph';
 import { toggleTypeDisabled } from '../actions/node-type';
 import reducer from '../reducers';
-import { graphNew, graphDagre } from '../utils/graph';
+import { graphNew } from '../utils/graph';
 import { sidebarWidth, largeGraphThreshold } from '../config';
 import animals from '../utils/data/animals.mock.json';
 import { getVisibleNodeIDs } from './disabled';
@@ -71,7 +71,7 @@ describe('Selectors', () => {
         () => toggleTypeDisabled('data', true),
         // Run layout to update state.graph
         (state) => {
-          const layout = state.flags.oldgraph ? graphDagre : graphNew;
+          const layout = graphNew;
           return updateGraph(layout(getGraphInput(state)));
         },
         // Turn the filter back off
@@ -92,7 +92,6 @@ describe('Selectors', () => {
           nodes: expect.any(Array),
           edges: expect.any(Array),
           layers: expect.any(Array),
-          oldgraph: expect.any(Boolean),
           fontLoaded: expect.any(Boolean),
         })
       );

src/selectors/metadata.js

@@ -88,7 +88,6 @@ export const getClickedNodeMetaData = createSelector(
       datasetType: nodeDatasetTypes[node.id],
     };
 
-    // Note: node.sources node.targets require oldgraph enabled
     if (node.sources && node.targets) {
       metadata.inputs = node.sources
         .map((edge) => nodes[edge.source])

src/store/initial-state.test.js

@@ -80,7 +80,7 @@ describe('prepareNonPipelineState', () => {
     // In this case, location.href is not provided
     expect(prepareNonPipelineState({ data: animals })).toMatchObject({
       flags: {
-        oldgraph: expect.any(Boolean),
+        newparams: expect.any(Boolean),
       },
     });
   });

src/utils/graph/index.js

@@ -1,4 +1,3 @@
-import dagre from 'dagre';
 import { graph } from './graph';
 
 /**
@@ -12,46 +11,5 @@ export const graphNew = ({ nodes, edges, layers }) => {
   return {
     ...result,
     size: { ...result.size, marginx: 100, marginy: 100 },
-    oldgraph: false,
-  };
-};
-
-/**
- * Calculate chart layout with Dagre.js.
- * This is an extremely expensive operation so we want it to run as infrequently
- * as possible, and keep it separate from other properties (like node.active)
- * which don't affect layout.
- */
-export const graphDagre = ({ nodes, edges, layers }) => {
-  const hasLayers = Boolean(layers.length);
-  const graph = new dagre.graphlib.Graph().setGraph({
-    ranker: hasLayers ? 'none' : null,
-    ranksep: hasLayers ? 200 : 70,
-    marginx: 40,
-    marginy: 40,
-  });
-
-  nodes.forEach((node) => {
-    graph.setNode(node.id, node);
-  });
-
-  edges.forEach((edge) => {
-    graph.setEdge(edge.source, edge.target, edge);
-  });
-
-  // Run Dagre layout to calculate X/Y positioning
-  dagre.layout(graph);
-
-  return {
-    nodes: graph.nodes().map((id) => {
-      const node = graph.node(id);
-      return {
-        ...node,
-        order: node.x + node.y * 9999,
-      };
-    }),
-    edges: graph.edges().map((id) => graph.edge(id)),
-    size: graph.graph(),
-    oldgraph: true,
   };
 };

src/utils/state.mock.js

@@ -9,7 +9,7 @@ import reducer from '../reducers';
 import { updateFontLoaded } from '../actions';
 import { getGraphInput } from '../selectors/layout';
 import { updateGraph } from '../actions/graph';
-import { graphNew, graphDagre } from './graph';
+import { graphNew } from './graph';
 
 /**
  * Prime the state object for the testing environment
@@ -32,7 +32,7 @@ export const prepareState = ({
     ...beforeLayoutActions,
     // Precalculate graph layout:
     (state) => {
-      const layout = state.flags.oldgraph ? graphDagre : graphNew;
+      const layout = graphNew;
       const graphState = getGraphInput(state);
       if (!graphState) {
         return state;