TS Sugestion(?) about d3-dag HOT 4 CLOSED

This is an interesting idea, and am open for discussion about how best to do this.

In terms of what you suggested, it's only really useful if you collect them into a list or some structure to keep the casting. The problem with that is that then this can happen:

const list = this.dag.descendants().map(node => {
    assert(isLayoutedNode(node));
    return node;
});
for (const node of dag) {
    node.x = undefined;
}
list[0].x; // has type `number`, but is `undefined`

Alternatively if you want to just use the x value, then your example can be done without any aliases or type guards with:

const list : number[] = this.dag.descendants().map(node => {
    assert(node.x !== undefined);
    return node.x;
});

If you look at old versions of the library (~0.4.0) sugiyama actually returned a cast of the current dag indicating that it was indeed laidout, e.g. x and y were also defined.

There was a problem getting this to work well. The way I originally implemented used polymorphic this, so that the children were "guaranteed" to be the same type, and therefore laidout. This proved to be more of a headache than it helped, so I got rid of it with a rewrite at some point, because getting types to work with polymorphic this. It also still has the same aliasing problem highlighted above.

Currently I just decided that typescripts non-null assertion was the way to go. It's simple, as safe as a type cast, and has no runtime overhead. Checking works, but if it's happening all the time it's wasting a lot time checking stuff we know to be true.

This ultimately leaves me thinking that exporting that type guard seems a little shallow and it's also pretty trivial to write if its useful to you. However, needed to do it still seems less than ideal. I see two potential routes, I'm not sure either is inherently worth doing.

1. There could be a dag / dag node utility class that wraps an existing dag instance and just checks property reads to guarantee that they're set in the underling dag. This would need to run each access, but would prevent you from writing it each time.
2. Remove x and y from dag, and instead have a subclass that does have x and y. Then unsugify can be set to just create that dag copy with guaranteed values. This prevents aliasing issues, and prevents having to check, but does require a copy and importantly delinks the returned dag from the input dag.

Thoughts?

from d3-dag.

Thoughts?

Not really, in my mind, you ask yourself the right questions ! :)
But that say I would be quite happy with option 2

from d3-dag.

I know it's been a long time since I last responded to this, I've been working on some massive rewrites to allow things to be more dynamic.

For many of the reasons outlined above, I don't think it really makes sense to add a new type definition. Originally I did work on a version that returned a new graph via unsugify. This would be fine the way the library currently works, but with the dynamic use case it makes things much more complex to still maintain d3 data binding, so I ultimately scrapped it. What I've chosen to go with is two separated properties:

1. a raw property that can be undefined called ux and uy
2. a corresponding getter and setter that only accepts numbers, and raises an exception if the properties are undefined

This isn't ideal in that is still requires runtime knowledge that the dag has been laid out, but you now no longer need non-null asserts, and it gives freedom in interface, e.g. you can still use ux and uy if you want to be safe.

Does this address your use case, or do you think there is still a better solution out there?

from d3-dag.

This standard is now implemented in the prerelease. x and y are always numbers, but will throw if the underlying ux or uy is undefined.

from d3-dag.