bug(ForceSimulation): Race condition in `linkPositions`, resulting in uninitialized state on first draw
Nobody has claimed this yet.
Assessment
- Difficulty
- 5/5
- Estimated time
- Over a week
- Newbie friendliness
- 35/100
- Issue type
- Bug
- Clarity
- Needs clarification
- Activity status
- Stale
- Tech stack
- d3js, typescript
- Domain
- data-visualization, frontend
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.
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
- Dominant language
- TypeScript
- Stars
- 1.3k
- Forks
- 41
- Avg merge
- 2d 8h
- Merged PRs (30d)
- 14
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.
More from techniq/layerchart
-
Difficulty 4/5 3-5 days Newbie friendliness 45/100
techniq/layerchart#919 ·
-
tab freezes when an awaited value feeding a chart changes (async svelte + kit remote functions) Open
Difficulty 5/5 Over a week Newbie friendliness 35/100
techniq/layerchart#895 · 4 comments ·
-
Difficulty 5/5 Over a week Newbie friendliness 38/100
techniq/layerchart#876 · 3 reactions ·
-
Difficulty 5/5 Over a week Newbie friendliness 35/100
techniq/layerchart#852 · 1 comment ·
-
Difficulty 4/5 3-5 days Newbie friendliness 48/100
techniq/layerchart#830 · 1 comment ·
All issues in techniq/layerchart
Similar issues
-
Difficulty 2/5 1-3 hours Newbie friendliness 84/100
Eynzof/Hermes-CN-Desktop#610 ·
-
bug clawsweeper:linked-pr-open clawsweeper:needs-live-repro clawsweeper:no-new-fix-pr impact:message-loss issue-rating: 🐚 platinum hermit P2 regression
Difficulty 2/5 1-3 hours Newbie friendliness 78/100
-
enhancement
Difficulty 2/5 1-3 hours Newbie friendliness 68/100
-
calcite-components needs triage refactor
Difficulty 2/5 1-3 hours Newbie friendliness 75/100
Esri/calcite-design-system#15203 ·
-
Difficulty 1/5 Under an hour Newbie friendliness 78/100
fullcalendar/fullcalendar#8106 ·