techniq / layerchart

Composable Svelte chart components to build a wide range of visualizations
https://www.layerchart.com
MIT License
668 stars 12 forks source link

`ForceSimulation` never evicts obsolete forces #206

Closed regexident closed 5 months ago

regexident commented 5 months ago

ForceSimulation currently passes its forces to its internal d3.Simulation<…> via simulation.force(name, force);. This has the unfortunate effect of forces never getting evicted from the internal d3.Simulation (i.e. simulation.force(name, null);), once removed from ForceSimulation's forces.

Object.entries(forces).forEach(([name, force]) => {
  simulation.force(name, force);
});

The only currently supported way to even say evict an existing force (e.g. center) is by doing this:

<ForceSimulation
  forces={{
    center: null,
    // other forces ...
  }}
>

… which, while working feels wrong. ForceSimulation's effective used forces should always mirror the current value of its forces prop.

regexident commented 5 months ago

A possible fix for this would be:

Add the following private prop to ForceSimulation:

// d3.Simulation does not provide a `.forces()` getter, so we need to
// keep track of previous forces ourselves, for diffing against `forces`.
let previousForces: Forces = {};

(I'm not aware of any other API for getting the before and after for a reactive change in Svelte)

… and then making use of the fact that all values within forces are objects (functions even) and thus must have reference semantics, thus replacing …

Object.entries(forces).forEach(([name, force]) => {
  simulation.force(name, force);
});

… with …

// Evict obsolete forces:
Object.keys(previousForces).forEach((name) => {
  if (!(name in forces)) {
    simulation.force(name, null);
  }
});

// Add new or overwrite existing forces:
Object.entries(forces).forEach(([name, force]) => {
  if (!(name in previousForces) || forces[name] !== previousForces[name]) {
    simulation.force(name, force);
  }
});

The fix above would also resolve #201.