Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TypeError: Cannot set properties of undefined (setting 'rank') #47

Open
ted-marozzi opened this issue Nov 26, 2024 · 10 comments · May be fixed by #49
Open

TypeError: Cannot set properties of undefined (setting 'rank') #47

ted-marozzi opened this issue Nov 26, 2024 · 10 comments · May be fixed by #49

Comments

@ted-marozzi
Copy link

I am getting this bug

file:///private/var/folders/n6/sq2t0xm10z362cvzt82x75jm0000gn/T/a6529bd28ad6c570b3c08f7bd0d5a535/src/dagre/rank/util.js:58
    return (label.rank = rank);
                       ^

TypeError: Cannot set properties of undefined (setting 'rank')
    at dfs (file:///private/var/folders/n6/sq2t0xm10z362cvzt82x75jm0000gn/T/a6529bd28ad6c570b3c08f7bd0d5a535/src/dagre/rank/util.js:58:24)
    at arrayEach (file:///private/var/folders/n6/sq2t0xm10z362cvzt82x75jm0000gn/T/a6529bd28ad6c570b3c08f7bd0d5a535/node_modules/lodash-es/_arrayEach.js:15:9)
    at Module.forEach (file:///private/var/folders/n6/sq2t0xm10z362cvzt82x75jm0000gn/T/a6529bd28ad6c570b3c08f7bd0d5a535/node_modules/lodash-es/forEach.js:38:10)
    at longestPath (file:///private/var/folders/n6/sq2t0xm10z362cvzt82x75jm0000gn/T/a6529bd28ad6c570b3c08f7bd0d5a535/src/dagre/rank/util.js:61:5)
    at networkSimplex (file:///private/var/folders/n6/sq2t0xm10z362cvzt82x75jm0000gn/T/a6529bd28ad6c570b3c08f7bd0d5a535/src/dagre/rank/network-simplex.js:52:3)
    at networkSimplexRanker (file:///private/var/folders/n6/sq2t0xm10z362cvzt82x75jm0000gn/T/a6529bd28ad6c570b3c08f7bd0d5a535/src/dagre/rank/index.js:51:3)
    at rank (file:///private/var/folders/n6/sq2t0xm10z362cvzt82x75jm0000gn/T/a6529bd28ad6c570b3c08f7bd0d5a535/src/dagre/rank/index.js:38:7)
    at file:///private/var/folders/n6/sq2t0xm10z362cvzt82x75jm0000gn/T/a6529bd28ad6c570b3c08f7bd0d5a535/src/dagre/layout.js:45:5
    at notime (file:///private/var/folders/n6/sq2t0xm10z362cvzt82x75jm0000gn/T/a6529bd28ad6c570b3c08f7bd0d5a535/src/dagre/util.js:249:10)
    at runLayout (file:///private/var/folders/n6/sq2t0xm10z362cvzt82x75jm0000gn/T/a6529bd28ad6c570b3c08f7bd0d5a535/src/dagre/layout.js:44:3)

Here is the minimal reproduction steps

import * as graphlibJson from "dagre-d3-es/src/graphlib/json.js";
import { layout } from "dagre-d3-es/src/dagre/index.js";

const g = graphlibJson.read({
  options: {
    directed: true,
    multigraph: true,
    compound: true,
  },
  nodes: [
    {
      v: "c",
      value: {
        id: "c",
        type: "group",

        height: 39,
      },
      parent: "b",
    },
    {
      v: "d",
      value: {
        id: "d",
        type: "group",
      },
      parent: "b",
    },
    {
      v: "f",
      value: {
        id: "f",
      },
      parent: "b",
    },
    {
      v: "e",
      value: {
        id: "e",
      },
      parent: "d",
    },
    {
      v: "b",
      value: {
        id: "b",
        type: "group",
      },
    },
  ],
  edges: [
    {
      v: "d",
      w: "f",
      name: "1",
      value: {
        style: "fill:none;",
        minlen: 1,
        labelpos: "c",
        id: "L-d-f-0",
      },
    },
  ],
  value: {
    rankdir: "LR",
    nodesep: 50,
    ranksep: 50,
    marginx: 8,
    marginy: 8,
  },
});

layout(g);
@ted-marozzi
Copy link
Author

For convenience, this is a code sandbox link demonstrating the issue. Start the code with npm run start

@ted-marozzi
Copy link
Author

ted-marozzi commented Nov 26, 2024

For what its worth this also causes an issue in mermaid with the following graph

flowchart
    subgraph a
        subgraph b
            subgraph d
                e
            end
            d --> f
        end
    end
    subgraph b
        subgraph c
        end
    end
Loading

@tbo47
Copy link
Owner

tbo47 commented Nov 27, 2024

Do you mind testing this https://github.com/tbo47/dagre-es/pull/48/files ?

You can do so by following this steps:

  • clone dagre-es
  • switch to the branch of the PR
  • npm link in the dagre-es folder
  • npm link dagre-es in your project
  • start your project and see if it fixes it

@ted-marozzi
Copy link
Author

There is another error

tedmarozzi@Teds-Personal-MacBook a6529bd28ad6c570b3c08f7bd0d5a535 %  node ./example/index.mjs
file:///private/var/folders/n6/sq2t0xm10z362cvzt82x75jm0000gn/T/a6529bd28ad6c570b3c08f7bd0d5a535/src/dagre/rank/util.js:64
  return g.node(e.w).rank - g.node(e.v).rank - g.edge(e).minlen;
                                       ^

TypeError: Cannot read properties of undefined (reading 'rank')
    at slack (file:///private/var/folders/n6/sq2t0xm10z362cvzt82x75jm0000gn/T/a6529bd28ad6c570b3c08f7bd0d5a535/src/dagre/rank/util.js:64:40)
    at file:///private/var/folders/n6/sq2t0xm10z362cvzt82x75jm0000gn/T/a6529bd28ad6c570b3c08f7bd0d5a535/src/dagre/rank/feasible-tree.js:59:29
    at arrayEach (file:///private/var/folders/n6/sq2t0xm10z362cvzt82x75jm0000gn/T/a6529bd28ad6c570b3c08f7bd0d5a535/node_modules/lodash-es/_arrayEach.js:15:9)
    at Module.forEach (file:///private/var/folders/n6/sq2t0xm10z362cvzt82x75jm0000gn/T/a6529bd28ad6c570b3c08f7bd0d5a535/node_modules/lodash-es/forEach.js:38:10)
    at dfs (file:///private/var/folders/n6/sq2t0xm10z362cvzt82x75jm0000gn/T/a6529bd28ad6c570b3c08f7bd0d5a535/src/dagre/rank/feasible-tree.js:56:7)
    at file:///private/var/folders/n6/sq2t0xm10z362cvzt82x75jm0000gn/T/a6529bd28ad6c570b3c08f7bd0d5a535/src/dagre/rank/feasible-tree.js:62:9
    at arrayEach (file:///private/var/folders/n6/sq2t0xm10z362cvzt82x75jm0000gn/T/a6529bd28ad6c570b3c08f7bd0d5a535/node_modules/lodash-es/_arrayEach.js:15:9)
    at Module.forEach (file:///private/var/folders/n6/sq2t0xm10z362cvzt82x75jm0000gn/T/a6529bd28ad6c570b3c08f7bd0d5a535/node_modules/lodash-es/forEach.js:38:10)
    at dfs (file:///private/var/folders/n6/sq2t0xm10z362cvzt82x75jm0000gn/T/a6529bd28ad6c570b3c08f7bd0d5a535/src/dagre/rank/feasible-tree.js:56:7)
    at file:///private/var/folders/n6/sq2t0xm10z362cvzt82x75jm0000gn/T/a6529bd28ad6c570b3c08f7bd0d5a535/src/dagre/rank/feasible-tree.js:62:9

Node.js v20.10.0

@tbo47
Copy link
Owner

tbo47 commented Nov 28, 2024

Do you mind testing this https://github.com/tbo47/dagre-es/pull/49/files

I tried to make it work here https://jsfiddle.net/tbo47/5093quxn/
There is no error but the rendering is not ok. Probably because it's rendering on a black background.

@ted-marozzi
Copy link
Author

ted-marozzi commented Nov 28, 2024

This almost worked, I just needed to add one more check to src/dagre/util.js

/*
 * Finds where a line starting at point ({x, y}) would intersect a rectangle
 * ({x, y, width, height}) if it were pointing at the rectangle's center.
 */
function intersectRect(rect, point) {
  if (!rect|| !point) return { x: 0, y: 0 };

@tbo47
Copy link
Owner

tbo47 commented Nov 29, 2024

added #49

@tbo47 tbo47 linked a pull request Nov 29, 2024 that will close this issue
@tbo47
Copy link
Owner

tbo47 commented Nov 29, 2024

I'm waiting for @aloisklink opinion and I can release a 7.0.12 if there is not regression in mermaid.
I would need to fix the unit tests first.

@aloisklink
Copy link
Collaborator

Sorry for the delay in getting back to you!

I've spent a few hours going through the dagre source-code and I'm very uncertain if #49 will fix it.

It looks like dagre does not support having an edge point to a group node, see:

I've had a try to fix it, but I've got no idea how. I tried adding dagrejs/dagre#293 and it seems to fix it, but it breaks other tests, so 🤷

Maybe what we could do is just add an exception to make it a bit more obvious? But I'll be honest, I don't really understand the code so I wouldn't know where to put it!

diff --git a/src/dagre/util.js b/src/dagre/util.js
index f526e82..9e9ee0d 100644
--- a/src/dagre/util.js
+++ b/src/dagre/util.js
@@ -60,6 +60,11 @@ function asNonCompoundGraph(g) {
     }
   });
   _.forEach(g.edges(), function (e) {
+    [e.v, e.w].forEach(function (v) {
+      if (!simplified.hasNode(v)) {
+        throw new Error('Compound node ' + v + ' has an edge, which is not supported');
+      }
+    });
     simplified.setEdge(e, g.edge(e));
   });
   return simplified;

I think #47 (comment) sounds more like a bug in Mermaid than in this library. Mermaid seems to have it's own logic for allowing edges to group/cluster nodes (see https://github.com/mermaid-js/mermaid/blob/6b87fb34189fa0e4ce39dde505339701912f77b0/packages/mermaid/src/dagre-wrapper/mermaid-graphlib.js#L204), and I think @ted-marozzi hit an edge-case in it.

@tbo47
Copy link
Owner

tbo47 commented Dec 6, 2024

@aloisklink thanks for taking the time to look at it.
Yeah, it's hard to come up with a patch when we know we can break other projects. This lib is widely used in production and we don't want to break things just for one edge case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants