d3-dag icon indicating copy to clipboard operation
d3-dag copied to clipboard

How to extend DagNode in vanilla js?

Open BenPortner opened this issue 1 year ago • 5 comments

Hi @erikbrinkman,

I am right now trying to migrate js_family_tree from d3-dag v.0.3.4 to the newest version. One problem I'm encountering is that DagNode does not appear as a class in the d3-dag.esm.js output. I need the class definition with all attributes and methods because I am extending it with my own FTNode class.

Things I've tried:

import { DagNode } from "./d3-dag/bundle/d3-dag.esm.min.js"; // doesn't work because no DagNode exported
import { DagNode } from "./d3-dag/src/index.ts"; // Firefox cannot load TypeScript modules
import { type DagNode } from "./d3-dag/src/index.ts"; // type cannot be used in Javascript

Any ideas how to fix this?

Thank you! Ben

BenPortner avatar Sep 07 '22 15:09 BenPortner

Unfortunately, at some point I intentionally hid the implementation of the DagNode class and only export a typescript interface, which is why you're having trouble extending it (I think this was before I kept keeping a changelog, so sorry if that wasn't more clear). I ultimately hid it because the class wasn't mean to extend, and nodes were meant to be static. This prevented invalid dags from being created manually.

I want to take a closer look at what you're doing to asses things better, but a few general comments:

First, I'm assuming given that you're extending the nodes, that you're also building your dag manually? Is there a reason why this is necessary versus encoding the different nodes types in the data attached to each node, and then just making functions that operate with respect to that data?

Second, due to many requests to better handle dynamism, I'm working (slowly unfortunately) on a rewrite that allows creation of general graphs. This should enable easier dynamic creation (rather than requiring using the static creation methods), and make it easier to use d3 data-binding to layout dynamic dags / graphs. However, in the rewrite I'm still planning on hiding actual node creation, and only exposing it via methods, so it won't be possible to subclass. Two points on this: first, it may make more sense just to wait for that change as it may fit your style more. Second: is there a reason I should try to enable subclassing? I'll look through what you're doing and maybe it will be obvious, but I'm mostly asking before I release something new to make sure it fits most use cases.

Sorry the update (seem to be) so painful. I'm really hoping to just cut v1 after the dynamic update and things should be much more stable from there.

erikbrinkman avatar Sep 08 '22 03:09 erikbrinkman

Hi @erikbrinkman,

First, I'm assuming given that you're extending the nodes, that you're also building your dag manually? Is there a reason why this is necessary versus encoding the different nodes types in the data attached to each node, and then just making functions that operate with respect to that data?

Yes, it is necessary. In fact, it's the whole point of js_family_tree! Instead of showing a huge family tree all at once, the user can discover it gradually by collapsing and extending parts of it. See the live example.

Second: is there a reason I should try to enable subclassing?

Let me reply with a question: Why should you prevent it? You say you want to prevent users from creating invalid dags. By users I believe you actually mean developers because a normal user would use the static methods and everything will work fine. Developers on the other hand will realize quickly if the dag they created is invalid and fix it accordingly. By preventing subclassing on the other hand, you are preventing development in the first place. This doesn't seem like a good trade-off to me 😉

BenPortner avatar Sep 08 '22 09:09 BenPortner

The live example is really cool!

From looking briefly at your code, it seems like you load the whole dag, and keep some variables for visibility. That visibility is then used to control traversal. On update you adjust visibility and then run the layout method again? That's essentially what this rewrite aims to do. To make dynamic building part of the design of the library. With that I should be able to tweak the layout algorithms to produce less jitter on successive layouts, and potentially make them faster for really large dags. This should also remove the need for custom subclassing to achieve something similar.

I am sympathetic to this argument in that it's better to empower other developers. However, people also abuse libraries and then ask for help as to why their abuse doesn't work, so preventing it is better. The notable reason to prevent subclassing is not that the subclassing is inherently bad, but that there's book keeping and other assumptions that are useful to make, and if someone creates a subclass and then violates those assumptions things can break is hard to debug ways.

Which makes me want to double down again. Assuming dynamic creation of nodes and edges, is there a reason to want to subclass? I still have some work to do, although I think the dag creation is working reasonably well. I'll take another look and see if there's an easy way to support giving more control in a yymv sense.

erikbrinkman avatar Sep 09 '22 02:09 erikbrinkman

Dear @erikbrinkman,

The live example is really cool!

Thank you! 😃

From looking briefly at your code, it seems like you load the whole dag, and keep some variables for visibility. That visibility is then used to control traversal. On update you adjust visibility and then run the layout method again?

Excatly.

That's essentially what this rewrite aims to do. To make dynamic building part of the design of the library. With that I should be able to tweak the layout algorithms to produce less jitter on successive layouts, and potentially make them faster for really large dags. This should also remove the need for custom subclassing to achieve something similar.

I find it hard to imagine a solution that will fit every user's/developer's needs (to be fair, I haven't tried very hard). And I am surprised that you would rather attempt this on your own rather than letting developers make the changes they need. But if the new version of d3-dag will be offering the functionality I need then I will be happy and thankful for it!

However, people also abuse libraries and then ask for help as to why their abuse doesn't work, so preventing it is better.

I don't have any experience with abuse, only with inexperienced usage. In this context trying to prevent people from asking questions seems questionable (pun intended). But then again, I never was the maintainer of a library that had 1200+ stars. I won't argue.

Which makes me want to double down again. Assuming dynamic creation of nodes and edges, is there a reason to want to subclass?

Maybe not. Here is my suggestion: Please let me know as soon as the new version is available. I will give it a spin. Meanwhile, I'll keep using v0.3.5.

BenPortner avatar Sep 09 '22 08:09 BenPortner

I am very sympathetic to let people do what they want, and seek to empower good developers. One of the problems with the current way you do things is that it doesn't work with progressively loading parts of the DAG since you need to have the whole one up front.

My ultimate reason to not allow subclassing is I don't have a good solution to allow it and enforce guarantees. But I will think on it more.

Your solution sounds good. Since it's a big change, I might start with a 1.0rc and let people try it before committing to something that isn't super viable. Thanks for the discussion!

erikbrinkman avatar Sep 11 '22 19:09 erikbrinkman