Rerendering keeps old node label size

[Layvier]

Hi,

first thanks a lot for this library, it's saving me a lot of time and pretty powerful !

So I'm rendering custom label react components, and in order to make it responsive I'm trying to update the nodes width based on the max number of nodes per rank. The issue is that after updating the nodes width, the layout doesn't update even though I'm properly changing the stage prop.

I'm assuming the issue comes from not resetting the ValueCache when the stage prop changes ?

If so I'm willing to make a PR, let me know what you think !

[bobthekingofegypt]

Hello, it's been a while since I looked at this as the project I built it for was cancelled a long time ago. But I'll try explain my understanding.

Currently there is no public api way to do this. I'm going on the assumption here that you want to do something like https://codesandbox.io/s/affectionate-grothendieck-znvsq?file=/src/App.tsx You want to force a dagre re-layout based on your sizing of existing nodes that have already been laid out. To do that valuecache won't help you, you need to trigger the useSize hook to re-measure your label, triggering it for label will bubble all the way and report the size change to graph that will relayout if everything is sized, to do that you need to cause one of these values to change https://github.com/bobthekingofegypt/dagre-reactjs/blob/master/src/useSize.ts#L51 for every node; unfortunatly labels don't use any of the optional arguments. The problem is I haven't given you a way to do this, in the codesandbox above I have cheated and relied on the fact I use debug labels attached to each node and you can invalidate those labels triggering the hook to run again but you shouldn't have to change the id's of your nodes and edges. That id change combined with a stage change will force a full layout. That's a lot of text.

I sort of made this library so it was difficult to impossible to accidentally trigger dagre layout calculations as we needed everything to be static and fast. Stage prop is intended only to cause a re-layout if you add or remove nodes, but it can also be used to allow updates to props to propagate to your nodes and labels for visual effects but will not run dagres layout again.

What I'm thinking is we need to add some way to invalidate the useSize hook whenever you want, something like the stage variable, that is passed in to the hook as a monitor. I think stage would have to stay separate so mouse over effects and other things do not trigger dagre again. This variable would have to be added to the renderNode function definition and the Node component and then into the useEffect hook signature somehow (https://github.com/bobthekingofegypt/dagre-reactjs/blob/master/src/Node.tsx#L35). Then if you incremented this value in theory it could force a layout for you.

You are welcome to try add this functionality, or I can maybe take a look at the weekend.

Cheers Bob

[Layvier]

@bobthekingofegypt Thanks for your answer! Makes sense indeed. I can look into it this weekend, seems quite straightforward. Regarding this variable name, what do you think of something like layoutStage then ? Cheers, Olivier

[bobthekingofegypt]

Sounds good.

[Layvier]

Hey, so I got it to work, I'm ready to make a PR. Can you please give the permissions to do so ? Also I had to run the linter because I couldn't commit without that. I also have a branch ready for a PR for that (would need to be merged first).

[bobthekingofegypt]

That's great. You should be able to make a PR from your own fork without commit permissions to this repository, or am I missing a setting on this repository?

[Layvier]

Oh yeah sorry my bad! I created the PRs (#10 and #11 ), let me know what you think.

[bobthekingofegypt]

It looks good, thanks for the feature. Sorry about the linter stuff, I should have tidied that up before. I'm going to merge them into a branch when I'm back at my main computer. I'll create a dev branch so it doesn't auto deploy the demo site.

There is one issue but it's not really with your code its with my chaining of useSize hooks. The issue means that when you invalidate the labels size a dagre layout will end up happening for each node in the current graph. So in the example you will get 5 layouts. I'm going to look at a solution to that before I release the next version.

Just setting undefined width and height on the nodes like stage does won't work as you need three render frames to have the sizing work its way up to node when you invalidate label.

Frame 1 - label with be laid out by browser Frame 2 - shape will re-adjust to new label size Frame 3 - node will re-adjust to new shape size

Problem is that unlike first render where the dom will report the size of node as 0 until the third render the dom will give its old size back to node on frame 2 causing the graph class to assume everything is sized and ready for layout. Then on frame 3 each nodes reportsize callback will also assume everything is sized and ready for layout. I'm hoping I can figure out a way to combine those calls somehow, at the moment I'm thinking the easiest solution is to store a copy of the size of each node in graph class and if nothing has changed don't relayout, it should be very fast maths even with 100's of nodes. It's actually the same reason I don't support preset width and height in node properties so the issue has always been there.

[Layvier]

Ok I see ! It looks quite complex so I'm not sure I can help. Keeping the node sizes in the graph class seems like a good option indeed, and checking for changes should be very fast. I'm pretty sure there's some optimization hacks for that, maybe by storing as string (JSON.stringify) and comparing the strings for instance. I know that it's one of the most efficient way of deep copying.

On an unrelated note, how difficult do you think it would be to support other graph layout algorithms than dagre ? I find myself a bit limited by it, and the dagre project seems quite inactive.

[bobthekingofegypt]

Do you know of an alternative I can look at?

[Layvier]

Yes, there's this one for instance: https://github.com/kieler/elkjs Another one: https://observablehq.com/@erikbrinkman/d3-dag-sugiyama

[bobthekingofegypt]

Well it wasn't something I had thought about, but since we are still in lockdown here so I had a play around today seeing how hard a change it would be. This is not the way I will do it in the end, this was just make it fit so I can see what needs to change but https://github.com/bobthekingofegypt/dagre-reactjs/tree/test/other-layouts branch has an example that is rendered with d3-dag, I even recreated their colorful demo.

I'm thinking I can give graph/graphd3dag a simple interface and maybe allow it's instance to be passed in to DagreReact as a prop. Rendering is almost entirely separate from the layout so it just involves making the data compatible and converting the results back. A lot more thought needs to be put into how to allow configuration, packaging, dep management but I'm pretty sure it can be done.

Elkjs will be interesting as its async so that will require more thought.

[Layvier]

Wow that's amazing, great job ! How about have a layout configuration prop looking like that ?

type LayoutConfiguration = {
    type: 'dagre',
    dependency: dagre,
    options: {...},
} | {
    type : 'd3-dag',
    dependency: d3dag,
    options: {...}
} | {
    type: 'custom',
    graphLayout: CustomGraphLayoutClass
}

It would allow safe typings on the dependencies and options. Peer dependencies could also be used and be installed independently, while throwing an error if it's missing.

Cytoscape js also has a custom layout feature (see https://js.cytoscape.org/#layouts and https://github.com/cytoscape/cytoscape.js-elk for the elk example), it could be good for inspiration.

[bobthekingofegypt]

Using a config like that is one of the options, (I haven't checked it in yet) but I'm starting to lean towards not including other layouts in this lib directly but just add the ability to do it, so they are in the examples project for now. Then release them as separate dependencies. I'm currently going back and forth between the ideas. React plus the automatic sizing makes it hard to come up with a nice react-y solution.

There will never be feature parity between them is another reason I'm thinking to leave them as third party support, d3-dag for example doesn't appear to support edge labels, elk doesn't appear to support per edge label configuration (who knows its documentation is impossible to comprehend). elk also supports a lot of things that aren't possible in my graph json format, or d3-dag/dagre layouting like nodes inside nodes.

If you don't mind saying what was dagre doing that was problematic? If I know I can keep it in mind when looking at d3-dag and elk implementations.

[Layvier]

Yeah I think I agree that the other layouts shouldn't be included in the library (it's named dagre-reactjs after all). I guess what we're talking about would be another library, independent of the chosen layout. Feature parity is then another issue for sure, I guess it would be more restricted.

Regarding my use case, I'm building a tool to create roadmaps collaboratively (here's an early version: https://sci-map.org/domains/web_design/goals/zyWNQIPPBd_ui_ux_essentials_for_frontend_developers). On the side, I'm working with the creator of https://github.com/andrico1234/beautiful-skill-tree to make an improved version of it, allowing children skills to have multiple parents for instance, which then makes a DAG data structure. As you can see on the showcase, there's a few issues with the layout:

• it could be more compact in terms of width
• I'd like it to push nodes "up" whenever possible, e.g. having the layout "Basics of UI design" be at the same rank as "Basics of UX design"
• I'd like to control the ordering of nodes within the same rank when there are several possibilities.

I think the main issue is that dagre isn't being improved anymore, some of the issues I'm pointing at are already requested. I'm considering forking it but DAGs rendering is a famously complex problem.

[bobthekingofegypt]

Sorry, work and things got in the way. Now I'm trying to clear some of my todo list.

I've created an alpha branch and an alpha release. The examples are auto deployed here https://amazing-sinoussi-60b78a.netlify.app/

Still undecided how best to do the layout interface, I've gone with inheritance for now but may change it over. I tried to look at allowing the issues you had to be addressed through the use of elk. Elk is a big complicated project with so many options and so little documentation I've barely had time to scratch its surface. I've made it so you can pass global options through the constructor, and node specific configuration though each nodes meta object to achieve your list. There is an example of me trying to deal with your list under elkordering, it doesn't really work perfectly, I'm sure there is a better way to do the ordering as this way is temperamental when you have two separate hierarchies. You can put numbers in the form and it will change the ranking of a node.

https://github.com/bobthekingofegypt/dagre-reactjs/blob/alpha/example/examples/elkordering/ElkOrdering.tsx There is now DAGReact and DagreReact; DAGReact allows you to instantiate your own graph layout. This can be seen in the above code, DagreReact should be backwards compatible with last release (mostly). There is a separate package dagre-reactjs-elk-layout that provides the elk layout code.

If you get a chance to have a look sometime let me know if it works for you, or if you need something changed.

[Layvier]

Hey, so sorry for the late reply, I didn't see the notification and was quite swamped with work. The Elk layout looks really great! It does seem more compact and of course I love the ability to change order. I'll play a bit more with it but I think it can be released, it solves most of my use cases. I really like the ability to provide a different graph layout as well.

Thanks a lot for taking the time to add all that, you rock 💪 !