vanjs-org / van

🍦 VanJS: World's smallest reactive UI framework. Incredibly Powerful, Insanely Small - Everyone can build a useful UI app in an hour.
https://vanjs.org
MIT License
3.77k stars 87 forks source link

Bug when deriving state inside an object #348

Closed nbonfils closed 1 month ago

nbonfils commented 1 month ago

I encountered a weird but that has me scratching my head...

Let's look at the following piece of code: 1721507157

If I uncomment the console.log statement the derivation runs properly each time settings.scalingRatio.value changes, but if I leave it commented like in the screenshot it doesn't run.

Any help or explaination as to what is happening would be highly appreciated!

Thanks for making van, it's really a great piece of software, been having a lot of fun making reactive webapps while being close to vanilla js!

Tao-VanJS commented 1 month ago

I think the line:

layout.settings.scalingRatio = logslider(layout.settings.scalingRatio.value.val)

is problematic.

Let's break down what this line does:

  1. Read the val property of the State object (let's name this State object s1) pointed by layout.settings.scalingRatio.value. By reading the val property, you're registering s1 as a dependency of the derivation.
  2. Call function logslider, which will create a new scalingRatio object, since the State object is in its value property, I assume a new State object will be created along the way (let's name this State object s2).

Thus, the end result is, you're registering s1 as the dependency of the derivation, but s1 might no longer be activity in your program (s2 is the one being actively used). In reality, if s1 is not being actively used, the dependency of it will subject to GC, whose behavior is subject to VanJS's internal implementation.

sirenkovladd commented 1 month ago

https://jsfiddle.net/jsg0ukqv/1/

@nbonfils, сould you please give more context, I have not been able to reproduce the error

nbonfils commented 1 month ago

Thank you both for your quick replies!

@Tao-VanJS Actually the line is:

layout.settings.scalingRatio = logslider(settings.scalingRatio.value.val);

We could even simplify it as:

layout.settings.scalingRatio = settings.scalingRatio.value.val;

So layout and settings are two different object, I tried renaming settings to config but to no avail as well.

@sirenkovladd That example looks pretty much like what I have, tomorrow I'll push it on a repo so I can share the full context of this code. In the meantime here is the full file: https://0x0.st/XpiX.js

As you can see I am currently working around the issue by doing this:

// Some code above ...

  let layout;
  let sigmaInstance;
  van.derive(() => {
    if (!graph.val) return;
    if (layout) layout.kill();
    layout = new FA2Layout(graph.val, {
      settings: {
        gravity: logslider(settings.gravity.value.rawVal),
        scalingRatio: logslider(settings.scalingRatio.value.rawVal),
      },
    });
    if (sigmaInstance) sigmaInstance.kill();
    sigmaInstance = new Sigma(graph.val, sigmaContainer, {
      itemSizesReference: "positions",
      zoomToSizeRatioFunction: (x) => x,
    });
    layout.start();
    layoutRunning.val = true;
  });

// More code in-between ...

  const settings = {
    scalingRatio: {
      value: van.state(70),
      min: 0,
      max: 100,
      label: "Scaling (overall graph size)",
    },
    gravity: {
      value: van.state(20),
      min: 0,
      max: 100,
      label: "Gravity (graph compactness)",
    },
    labelSize: {
      value: van.state(14),
      min: 0,
      max: 100,
      label: "Label size",
    },
    labelDensity: {
      value: van.state(1),
      min: 0,
      max: 10,
      label: "Label density",
    },
    labelRenderThreshold: {
      value: van.state(6),
      min: 0,
      max: 10,
      label: "Label render threshold",
    },
    nodeScale: {
      value: van.state(100),
      min: 1,
      max: 200,
      label: "Node scale",
    },
  };

  van.derive(() => {
    settings.scalingRatio.value.val;
    if (layout)
      layout.settings.scalingRatio = logslider(settings.scalingRatio.value.val);
  });
  van.derive(() => {
    settings.gravity.value.val;
    if (layout) layout.settings.gravity = logslider(settings.gravity.value.val);
  });
  van.derive(() => {
    settings.labelSize.value.val;
    sigmaInstance?.setSetting("labelSize", settings.labelSize.value.val);
  });
  van.derive(() => {
    settings.labelDensity.value.val;
    sigmaInstance?.setSetting("labelDensity", settings.labelDensity.value.val);
  });
  van.derive(() => {
    settings.labelRenderThreshold.value.val;
    sigmaInstance?.setSetting(
      "labelRenderedSizeThreshold",
      settings.labelRenderThreshold.value.val,
    );
  });
  van.derive(() => {
    settings.nodeScale.value.val;
    graph.rawVal?.forEachNode((nodeRef, attr) =>
      graph.rawVal.mergeNodeAttributes(nodeRef, {
        size: attr.origSize * (settings.nodeScale.value.val / 100),
      }),
    );
  });

// Even more code below ...

Each of those derive statement don't run without the explicit val access from the state.

Tao-VanJS commented 1 month ago

Ah..., I see.

For the derivation here:

  van.derive(() => {
    // Without this line: settings.scalingRatio.value.val;
    if (layout)
      layout.settings.scalingRatio = logslider(settings.scalingRatio.value.val);
  });

layout needs to be set when van.derive is being called. Otherwise, settings.scalingRatio.value.val won't be read, thus settings.scalingRatio.value won't be registered as a dependency of the derivation.

nbonfils commented 1 month ago

Yes, so layout is undefined initially, but then once layout is defined and I move the slider that changes settings.scalingRatio.value.val the derivation is called only when the first line is uncommented, if I change my code to reflect your example, then the derivation is never called. That's exactly what is having me puzzled.

Tao-VanJS commented 1 month ago

Let's take a look at the example below:

let layout = undefined;

van.derive(() => {
  // Without this line: settings.scalingRatio.value.val;
  if (layout)
    layout.settings.scalingRatio = logslider(settings.scalingRatio.value.val);
});

When van.derive is called. The "then clause" of the if statement is not called. As a result, settings.scalingRatio.value.val is not accessed, thus the state settings.scalingRatio.value is not registered as a dependency of the derivation. Future changes of settings.scalingRatio.value will not trigger the derivation.

If the code is like that:

let layout = undefined;

van.derive(() => {
  settings.scalingRatio.value.val;
  if (layout)
    layout.settings.scalingRatio = logslider(settings.scalingRatio.value.val);
});

settings.scalingRatio.value.val is accessed explicitly in the first line of derivation, thus settings.scalingRatio.value is registered as a dependency of derivation in this implementation.

nbonfils commented 1 month ago

Ahh that makes sense thank you for this explaination!

But what about this other case:

van.derive(() => {
    // settings.labelSize.value.val;
    sigmaInstance?.setSetting("labelSize", settings.labelSize.value.val);
  });

With settings.labelSize.value.val; commented out, this derive statement doesn't trigger when settings.labelSize.value.val changes. Why is that the case here?

Forgive me if I missed this in the docs but does the "registration" occur when the function inside the van.derive statement runs for the first time?

Tao-VanJS commented 1 month ago
sigmaInstance?.setSetting("labelSize", settings.labelSize.value.val)

is just the syntax sugar of

if (sigmaInstance)
  sigmaInstance.setSetting("labelSize", settings.labelSize.value.val)

Every time when the derivation function is called, all the states accessed in the derivation will be registered as the dependencies of derivation. When any of the dependency states changes, the derivation will be triggered, and a new set of dependencies will be registered upon the execution of the derivation.

I think the example in this section illustrates the mechanism well.

nbonfils commented 1 month ago

Got it! So the derive statement not triggering stems from the fact that that sigmaInstance is not a state, therefore not registered and won't retrigger the derive statement when it changes and is available, thus never registering settings.labelSize.value.val.

Am I understanding this correctly?

So another way to go about this would be to have another state, let's call it sigmaInstanceAvailable and do the following:

van.derive(() => {
  if (sigmaInstanceAvailable.val)
    sigmaInstance.setSetting("labelSize", settings.labelSize.value.val)
});
sirenkovladd commented 1 month ago

Yes, that would be a very nice optimized code

Tao-VanJS commented 1 month ago
van.derive(() => {
  if (sigmaInstanceAvailable.val)
    sigmaInstance.setSetting("labelSize", settings.labelSize.value.val)
});

Alternatively, you can make sigmaInstance a State object. Then you can simply have:

van.derive(() => {
  sigmaInstance.val?.setSetting("labelSize", settings.labelSize.value.val)
})
nbonfils commented 1 month ago

Thank you very much for taking the time to explain it to me! Closing the issue since it's not a bug.

And I don't know if it's of interest to you to see where van is used, but here is the project that we are going to release in the coming weeks: https://github.com/nbonfils/bibliograph2

We switched from Alpine to Van and I am not looking back, it's honestly a great framework when you don't want to deal with the absurd complexity of everything else out there!