Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug when deriving state inside an object #348

Closed
nbonfils opened this issue Jul 20, 2024 · 12 comments
Closed

Bug when deriving state inside an object #348

nbonfils opened this issue Jul 20, 2024 · 12 comments

Comments

@nbonfils
Copy link

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
Copy link
Member

Tao-VanJS commented Jul 20, 2024

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
Copy link
Contributor

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

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

@nbonfils
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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
Copy link
Author

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
Copy link
Contributor

Yes, that would be a very nice optimized code

@Tao-VanJS
Copy link
Member

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
Copy link
Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants