Skip to content

bug(ForceSimulation): Race condition in linkPositions, resulting in uninitialized state on first draw #553

@regexident

Description

@regexident

As a result the linkPositions snippet arguments is still empty ([]) on the very first draw.

This results in linkPositions[index] producing undefined, which is rather unexpected (and took me a while to figure out what was causing my code to crash).

The reason for this bug (afaict) is that linkPositions remains [] until the very first onTick. This however is always to late for the first draw and for static (or stopped) simulations it never happens at all. This can easily be observed by the graph layout of the ForceGraph example breaking, as soon as you check the static checkbox: https://next.layerchart.com/docs/examples/ForceGraph.

Obtaining a reasonable initial value for linkPositions isn't entirely trivial though:

SimulationLinkDatum<Node>'s source/target properties are of type string | number | NodeDatum:

export interface SimulationLinkDatum<NodeDatum extends SimulationNodeDatum> {
    /**
     * Link’s source node.
     * For convenience, a link’s source and target properties may be initialized using numeric or string identifiers rather than object references; see link.id.
     * When the link force is initialized (or re-initialized, as when the nodes or links change), any link.source or link.target property which is not an object
     * is replaced by an object reference to the corresponding node with the given identifier.
     * After initialization, the source property represents the source node object.
     */
    source: NodeDatum | string | number;
    /**
     * Link’s source link
     * For convenience, a link’s source and target properties may be initialized using numeric or string identifiers rather than object references; see link.id.
     * When the link force is initialized (or re-initialized, as when the nodes or links change), any link.source or link.target property which is not an object
     * is replaced by an object reference to the corresponding node with the given identifier.
     * After initialization, the target property represents the target node object.
     */
    target: NodeDatum | string | number;
    /**
     * The zero-based index into the links array. Internally generated when calling ForceLink.links(...)
     */
    index?: number | undefined;
}

The link's source/target properties don't get resolved to NodeDatum, until the forces have been initialized.
The earliest time we even pass forces to the internal simulation however (and thus triggering initialization) is here:

watch.pre(
  () => forces,
  () => {
    // Any time the `forces` prop gets changed we
    // pass them to the internal d3 simulation object:
    pushForcesToSimulation(forces);
    runOrResumeSimulation();
  }
);

… which I would assume happens strictly after this:

let linkPositions: LinkPosition[] = $state([]);

So even if we replaced that line with something like this:

let linkPositions: LinkPosition[] = $state((data.links ?? []).map((link) => ({
  x1: link.source.x,
  y1: link.source.y,
  x2: link.target.x,
  y2: link.target.y
})));

… the state of the source/target fields of the elements in data.links would still be uninitialized.

And even if we had access to initialized links' source/target nodes:

The x,y properties of SimulationNodeDatum are number | undefined and if the simulation hasn't "ticked" yet, then those might still be undefined (very likely so even). So one would have to either make the properties on LinkPosition be number | undefined either (which would result in rather bad ergonomics), or define a fallback default, which would probably be 0:

{
  x1: link.source.x ?? 0.0,
  y1: link.source.y ?? 0.0,
  x2: link.target.x ?? 0.0,
  y2: link.target.y ?? 0.0
}

… but in many scenarios a default position of 0,0 is actually undesired.

In my experience a default fallback value of width/2, height/2 would usually be preferred. But that opens yet another can of worms, since there would be plenty of scenarios where even width/2, height/2 would be inadequate, not to mention that the element width/height might not even be known before the first actual DOM draw.

This feels somewhat gnarly and inherently brittle and prone to race conditions. I wonder if there's another way to address the root issue that linkPositions is meant to solve?

cc @huntabyte

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions