Cleanup sankey diagrams according code review

demos/sankey.html

@@ -21,7 +21,6 @@
       
       Agricultural 'waste',Bio-conversion,124.729
       Bio-conversion,Liquid,0.597
-      %% line with comment
       Bio-conversion,Losses,26.862
       Bio-conversion,Solid,280.322
       Bio-conversion,Gas,81.144

packages/mermaid/src/diagrams/sankey/parser/desired_syntax.md

@@ -1,154 +0,0 @@
-# Sankey diagrams syntax proposal
-
-## What is used now
-
-**Circular graphs are not supported by d3. There are some alternatives for that.**
-**Dropped flows are not supported by d3**
-
-This is example of data for Sakey diagrams from d3 author (simple csv):
-
-```csv
-Berlin,Job Applications,102
-Barcelona,Job Applications,39
-Madrid,Job Applications,35
-Amsterdam,Job Applications,15
-Paris,Job Applications,14
-London,Job Applications,6
-Munich,Job Applications,5
-Brussels,Job Applications,4
-Dubai,Job Applications,3
-Dublin,Job Applications,3
-Other Cities,Job Applications,12
-Job Applications,No Response,189
-Job Applications,Responded,49,orange
-Responded,Rejected,38
-Responded,Interviewed,11,orange
-Interviewed,No Offer,8
-Interviewed,Declined Offer,2
-Interviewed,Accepted Offer,1,orange
-```
-
-GoJS uses similar approach:
-
-```json
-{
-"nodeDataArray": [
-{"key":"Coal reserves", "text":"Coal reserves", "color":"#9d75c2"},
-{"key":"Coal imports", "text":"Coal imports", "color":"#9d75c2"},
-{"key":"Oil reserves", "text":"Oil\nreserves", "color":"#9d75c2"},
-{"key":"Oil imports", "text":"Oil imports", "color":"#9d75c2"}
-],
-"linkDataArray": [
-{"from":"Coal reserves", "to":"Coal", "width":31},
-{"from":"Coal imports", "to":"Coal", "width":86},
-{"from":"Oil reserves", "to":"Oil", "width":244}
-}
-```
-
-## What do we need
-
-Mainly we need:
-
-- collection of nodes
-- collection of links
-
-We also need graph and node attributes like this:
-
-- link sort
-- node sort
-- coloring strategy for links (source, target, transition)
-- graph alignment (left, right, width)
-- node color
-- node title
-- node width
-- node padding
-- graph margin
-
-## Desired syntax
-
-Graph is a list of flows (or links).
-Flow is a sequence `node -> value -> node -> value...`
-
-```
-a -> 30 -> b
-a -> 40 -> b
-```
-
-2 separate streams between 2 nodes they can be grouped as well:
-
-```
-a -> {
-    30
-    40
-} -> b
-```
-
-All outflows from the node can be grouped:
-
-```
-a -> {
-    30 -> b
-    40 -> c
-}
-```
-
-All inflows to the node can be grouped too:
-
-```
-{
-    a -> 30
-    b -> 40
-} -> c
-```
-
-Chaining example:
-
-```
-a -> {
-    30
-    40
-} -> b -> {
-    20 -> d -> 11
-    50 -> e -> 11
-} -> f -> 30
-```
-
-**Probably ambiguous!**
-
-Does the sample below mean that total outflow from "a" is 60?
-
-```
-a -> 30 -> {
-    b
-    c
-}
-```
-
-Or does this one mean that total outflow must be 140 (70 to "b" and "c" respectively)?
-
-```
-a -> {
-    30
-    40
-} -> {
-    b
-    c
-}
-```
-
-**Overcomplicated**
-
-Nested:
-
-```
-{
-    {
-        a -> 30
-        b -> 40
-    } -> c -> 12
-    {
-        d -> 10
-        e -> 20
-    } -> f -> 43
-} -> g
-```

packages/mermaid/src/diagrams/sankey/parser/sankey-arrow.spec.js

@@ -85,7 +85,7 @@ describe('Sankey diagram', function () {
         parser.parse(str);
       });
 
-      it('parses truly complex example', () => {
+      it('parses real example', () => {
         const str = `
         sankey
 
@@ -95,68 +95,6 @@ describe('Sankey diagram', function () {
         "Bio-conversion"            ->      280.322  -> "Solid"
         "Bio-conversion"            ->      81.144   -> "Gas"
         "Biofuel imports"           ->      35       -> "Liquid"
-        "Biomass imports"           ->      35       -> "Solid"
-        "Coal imports"              ->      11.606   -> "Coal"
-        "Coal reserves"             ->      63.965   -> "Coal"
-        "Coal"                      ->      75.571   -> "Solid"
-        "District heating"          ->      10.639   -> "Industry"
-        "District heating"          ->      22.505   -> "Heating and cooling - commercial"
-        "District heating"          ->      46.184   -> "Heating and cooling - homes"
-        "Electricity grid"          ->      104.453  -> "Over generation / exports"
-        "Electricity grid"          ->      113.726  -> "Heating and cooling - homes"
-        "Electricity grid"          ->      27.14    -> "H2 conversion"
-        "Electricity grid"          ->      342.165  -> "Industry"
-        "Electricity grid"          ->      37.797   -> "Road transport"
-        "Electricity grid"          ->      4.412    -> "Agriculture"
-        "Electricity grid"          ->      40.858   -> "Heating and cooling - commercial"
-        "Electricity grid"          ->      56.691   -> "Losses"
-        "Electricity grid"          ->      7.863    -> "Rail transport"
-        "Electricity grid"          ->      90.008   -> "Lighting & appliances - commercial"
-        "Electricity grid"          ->      93.494   -> "Lighting & appliances - homes"
-        "Gas imports"               ->      40.719   -> "Ngas"
-        "Gas reserves"              ->      82.233   -> "Ngas"
-        "Gas"                       ->      0.129    -> "Heating and cooling - commercial"
-        "Gas"                       ->      1.401    -> "Losses"
-        "Gas"                       ->      151.891  -> "Thermal generation"
-        "Gas"                       ->      2.096    -> "Agriculture"
-        "Gas"                       ->      48.58    -> "Industry"
-        "Geothermal"                ->      7.013    -> "Electricity grid"
-        "H2 conversion"             ->      20.897   -> "H2"
-        "H2 conversion"             ->      6.242    -> "Losses"
-        "H2"                        ->      20.897   -> "Road transport"
-        "Hydro"                     ->      6.995    -> "Electricity grid"
-        "Liquid"                    ->      121.066  -> "Industry"
-        "Liquid"                    ->      128.69   -> "International shipping"
-        "Liquid"                    ->      135.835  -> "Road transport"
-        "Liquid"                    ->      14.458   -> "Domestic aviation"
-        "Liquid"                    ->      206.267  -> "International aviation"
-        "Liquid"                    ->      3.64     -> "Agriculture"
-        "Liquid"                    ->      33.218   -> "National navigation"
-        "Liquid"                    ->      4.413    -> "Rail transport"
-        "Marine algae"              ->      4.375    -> "Bio-conversion"
-        "Ngas"                      ->      122.952  -> "Gas"
-        "Nuclear"                   ->      839.978  -> "Thermal generation"
-        "Oil imports"               ->      504.287  -> "Oil"
-        "Oil reserves"              ->      107.703  -> "Oil"
-        "Oil"                       ->      611.99   -> "Liquid"
-        "Other waste"               ->      56.587   -> "Solid"
-        "Other waste"               ->      77.81    -> "Bio-conversion"
-        "Pumped heat"               ->      193.026  -> "Heating and cooling - homes"
-        "Pumped heat"               ->      70.672   -> "Heating and cooling - commercial"
-        "Solar PV"                  ->      59.901   -> "Electricity grid"
-        "Solar Thermal"             ->      19.263   -> "Heating and cooling - homes"
-        "Solar"                     ->      19.263   -> "Solar Thermal"
-        "Solar"                     ->      59.901   -> "Solar PV"
-        "Solid"                     ->      0.882    -> "Agriculture"
-        "Solid"                     ->      400.12   -> "Thermal generation"
-        "Solid"                     ->      46.477   -> "Industry"
-        "Thermal generation"        ->      525.531  -> "Electricity grid"
-        "Thermal generation"        ->      787.129  -> "Losses"
-        "Thermal generation"        ->      79.329   -> "District heating"
-        "Tidal"                     ->      9.452    -> "Electricity grid"
-        "UK land based bioenergy"   ->      182.01   -> "Bio-conversion"
-        "Wave"                      ->      19.013   -> "Electricity grid"
-        "Wind"                      ->      289.366  -> "Electricity grid"
         `;
 
         parser.parse(str);

packages/mermaid/src/diagrams/sankey/parser/sankey.spec.ts

@@ -4,10 +4,9 @@ import diagram from './sankey.jison';
 import { parser } from './sankey.jison';
 import db from '../sankeyDB.js';
 import { cleanupComments } from '../../../diagram-api/comments.js';
-import { prepareTextForParsing } from '../sankeyDiagram.js';
+import { prepareTextForParsing } from '../sankeyUtils.js';
 
 describe('Sankey diagram', function () {
-  // TODO - these examples should be put into ./parser/stateDiagram.spec.js
   describe('when parsing an info graph it', function () {
     beforeEach(function () {
       parser.yy = db;
@@ -20,13 +19,7 @@ describe('Sankey diagram', function () {
       const path = await import('path');
       const csv = path.resolve(__dirname, './energy.csv');
       const data = fs.readFileSync(csv, 'utf8');
-
-      // Add \n\n + space to emulate possible possible imperfections
       const graphDefinition = prepareTextForParsing(cleanupComments('sankey\n\n ' + data));
-      // const textToParse = graphDefinition
-      //   .replaceAll(/^[^\S\r\n]+|[^\S\r\n]+$/g, '')
-      //   .replaceAll(/([\n\r])+/g, "\n")
-      //   .trim();
 
       parser.parse(graphDefinition);
     });

packages/mermaid/src/diagrams/sankey/sankeyDB.ts

@@ -1,5 +1,3 @@
-// import { log } from '../../logger.js';
-// import mermaidAPI from '../../mermaidAPI.js';
 import * as configApi from '../../config.js';
 import common from '../common/common.js';
 import {
@@ -19,11 +17,13 @@ import {
 let links: Array<SankeyLink> = [];
 let nodes: Array<SankeyNode> = [];
 let nodesHash: Record<string, SankeyNode> = {};
-let nodeAlign: string = 'justify';
+let nodeAlign = 'justify';
 
 const setNodeAlign = function (alignment: string): void {
   const nodeAlignments: string[] = ['left', 'right', 'center', 'justify'];
-  if (nodeAlignments.includes(alignment)) nodeAlign = alignment;
+  if (nodeAlignments.includes(alignment)) {
+    nodeAlign = alignment;
+  }
 };
 
 const getNodeAlign = () => nodeAlign;
@@ -96,7 +96,6 @@ export default {
   findOrCreateNode,
   setNodeAlign,
   getNodeAlign,
-  // TODO: If this is a must this probably should be an interface
   getAccTitle,
   setAccTitle,
   getAccDescription,

packages/mermaid/src/diagrams/sankey/sankeyDiagram.ts

@@ -5,18 +5,6 @@ import db from './sankeyDB.js';
 import styles from './styles.js';
 import renderer from './sankeyRenderer.js';
 
-export const prepareTextForParsing = (text: string): string => {
-  const textToParse = text
-    .replaceAll(/^[^\S\r\n]+|[^\S\r\n]+$/g, '') // remove all trailing spaces for each row
-    .replaceAll(/([\n\r])+/g, '\n') // remove empty lines duplicated
-    .trim();
-
-  return textToParse;
-};
-
-const originalParse = parser.parse.bind(parser);
-parser.parse = (text: string) => originalParse(prepareTextForParsing(text));
-
 export const diagram: DiagramDefinition = {
   parser,
   db,

packages/mermaid/src/diagrams/sankey/sankeyRenderer.ts

@@ -1,14 +1,11 @@
 // @ts-nocheck TODO: fix file
 import { Diagram } from '../../Diagram.js';
-// import { log } from '../../logger.js';
 import * as configApi from '../../config.js';
 
 import {
   select as d3select,
   scaleOrdinal as d3scaleOrdinal,
   schemeTableau10 as d3schemeTableau10,
-  // rgb as d3rgb,
-  // map as d3map,
 } from 'd3';
 
 import {
@@ -20,7 +17,7 @@ import {
   sankeyJustify as d3SankeyJustify,
 } from 'd3-sankey';
 import { configureSvgSize } from '../../setupGraphViewbox.js';
-// import { debug } from 'console';
+import { prepareTextForParsing } from './sankeyUtils.js';
 
 /**
  * Draws a sequenceDiagram in the tag with id: id based on the graph definition in text.
@@ -31,20 +28,15 @@ import { configureSvgSize } from '../../setupGraphViewbox.js';
  * @param diagObj - A standard diagram containing the db and the text and type etc of the diagram
  */
 export const draw = function (text: string, id: string, _version: string, diagObj: Diagram): void {
-  // First of all parse sankey language
+  // Clear DB before parsing
-  // Everything that is parsed will be stored in diagObj.DB
-  // That is why we need to clear DB first
   //
-  if (diagObj?.db?.clear !== undefined) {
+  diagObj.db.clear?.();
-    // why do we need to check for undefined? typescript complains
+  
-    diagObj?.db?.clear();
-  }
   // Launch parsing
-  diagObj.parser.parse(text);
+  const preparedText = prepareTextForParsing(text);
-  // debugger;
+  diagObj.parser.parse(preparedText);
-  // log.debug('Parsed sankey diagram');
 
-  // Figure out what is happening there
+  // TODO: Figure out what is happening there
   // The main thing is svg object that is a d3 wrapper for svg operations
   //
   const { securityLevel, sequence: conf } = configApi.getConfig();
@@ -95,10 +87,10 @@ export const draw = function (text: string, id: string, _version: string, diagOb
   };
   const nodeAlign = nodeAligns[diagObj.db.getNodeAlign()];
 
-  const nodeWidth = 10;
   // Construct and configure a Sankey generator
   // That will be a function that calculates nodes and links dimensions
   //
+  const nodeWidth = 10;
   const sankey = d3Sankey()
     .nodeId((d) => d.id) // we use 'id' property to identify node
     .nodeWidth(nodeWidth)
@@ -109,10 +101,8 @@ export const draw = function (text: string, id: string, _version: string, diagOb
       [width - nodeWidth, height],
     ]);
 
-  // Compute the Sankey layout
+  // Compute the Sankey layout: calculate nodes and links positions
-  // Namely calculate nodes and links positions
+  // Our `graph` object will be mutated by this and enriched with other properties
-  // Our `graph` object will be mutated by this
-  // and enriched with some properties
   //
   sankey(graph);
 

packages/mermaid/src/diagrams/sankey/sankeyUtils.ts

@@ -0,0 +1,8 @@
+export const prepareTextForParsing = (text: string): string => {
+  const textToParse = text
+    .replaceAll(/^[^\S\n\r]+|[^\S\n\r]+$/g, '') // remove all trailing spaces for each row
+    .replaceAll(/([\n\r])+/g, '\n') // remove empty lines duplicated
+    .trim();
+
+  return textToParse;
+};

packages/mermaid/src/setupGraphViewbox.js

@@ -25,7 +25,6 @@ export const calculateSvgSizeAttrs = function (height, width, useMaxWidth) {
   if (useMaxWidth) {
     attrs.set('width', '100%');
     attrs.set('style', `max-width: ${width}px;`);
-    // TODO: when using max width it does not set height? Is it intended?
   } else {
     attrs.set('height', height);
     attrs.set('width', width);