bug(ForceSimulation): Race condition in `linkPositions`, resulting in uninitialized state on first draw
Nobody has claimed this yet.
- Dominant language
- TypeScript
- Stars
- 1.3k
- Forks
- 41
- Avg merge
- 2d 19h
- Merged PRs (30d)
- 16
Description
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
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the ForceSimulation code around the $state([]) initialization of linkPositions and the watch.pre callback that calls pushForcesToSimulation and runOrResumeSimulation. Reproduce the static ForceGraph example, then trace the first draw before onTick; done means linkPositions is valid on the initial draw without relying on a tick.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- d3js, typescript
- Domain
- data-visualization, frontend
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 35/100