Derive working with single derived property, but failing with two

[DavidMulder0]

Summary

Have a hard time describing the issue, because if I had figured out what's exactly going on I would be doing a pull request instead of a bug report, however I do have a decently clean MVE:

const createItem = (id: string, store: any): any => {
  let itemProxy = proxy({
    id
  });
  derive(
    {
      // commenting out the `isSelected` derived property magically causes the `isHovered` property to work
      isSelected: (get) => {
        return get(store).selected === get(itemProxy).id;
      },
      isHovered: (get) => {
        return get(store).hovered === get(itemProxy).id;
      }
    },
    { proxy: itemProxy }
  );
  return itemProxy;
};

const store = proxy({
  selected: (undefined as unknown) as string,
  hovered: (undefined as unknown) as string,
  byId: {} as Record<
    string,
    {
      id: string;
      readonly isHovered: boolean;
      readonly isSelected: boolean;
    }
  >
});

store.byId.a = createItem("a", store);
store.byId.b = createItem("b", store);

const nextTick = (t = 0) => new Promise((resolve) => setTimeout(resolve, t));

console.group("setting hovered to item A");
store.hovered = "a";
await nextTick();
console.log("a.isHovered", store.byId.a.isHovered); // true
console.log("b.isHovered", store.byId.b.isHovered); // false
console.groupEnd();

console.group("setting hovered to item B");
store.hovered = "b";
await nextTick();
console.log("a.isHovered", store.byId.a.isHovered); // false
console.log("b.isHovered", store.byId.b.isHovered); // false, should be true
console.groupEnd();

Link to reproduction

https://codesandbox.io/s/laughing-gates-mo58dj?file=/src/index.ts

[DavidMulder0]

For whatever it is worth, when sync: true is passed to the derive the issue seems to not get triggered.

[dai-shi]

Hmmm, it sounds like a bug. Probably a tough one. Looks like there are two derived states with circular structure.

Hope someone can dig into it.

[DavidMulder0]

Still trying to wrap my head around what's happening, but just going to share what I got:

After initialization of the store the pendingCount in the sourceObjectEntry for byId.b ends up as -4.

store.byId.a = createItem('a', store);
store.byId.b = createItem('b', store);

await time();

console.log(
  sourceObjectMap.get(store.byId.b).pendingCount
); // -4

What seems to happen is that at the time of the store.byId.a assignment store.byId.b of course does not yet exist, so markPending only gets executed for the already existing subscriptions. However once the unmarkPending gets executed for the store.byId.a assignment on the next tick, the store.byId.b assignment already happened, thus unmarkPending gets executed a bunch of extra times without a corresponding markPending call ever happening.

Execution order as far as I understand it is:

1) store.byId.a assignment 2) markPending store.byId.a only 3) store.byId.b assignment 4) unmarkPending BOTH store.byId.a and store.byId.b

Thus waiting a tick between store.byId.a = and store.byId.b = causes the issue not to trigger.

store.byId.a = createItem('a', store);
await time();
store.byId.b = createItem('b', store);

A solution that might work is to make a shallow copy of the subscriptions before the subscription.promise = Promise.resolve().then, but I am far from sure whether that wouldn't have other unintended consequences. Definitely need to check out the entire library and run the unit tests against it at the very least, but would love some feedback whether that does or doesn't sound reasonable.

[DavidMulder0]

Plus, I haven't yet mentally modelled out why commenting out the isSelected derived property made the isHovered derived property work, so that's something I will have to look into as well~

[ssijak]

@DavidMulder0 Hey, have you figured this out? I am experiencing the same issue and its pretty hard to wrap the head around the source code, would be helpful if you can give me a headstart

[CaptainStiggz]

I have the same problem as well! After a VERY cursory look, I'm not sure what the point of running without {sync: true} is. It seems like the difference is just:

if (notifyInSync) {
  unmarkPending(sourceObject)
} else {
  subscription.p = Promise.resolve().then(() => {
    delete subscription.p // promise
    unmarkPending(sourceObject)
  })
}

What's the objective of {sync: false}?

[dai-shi]

With sync, batching is disabled, which causes too many updates in some cases, possibly dropping performance.

[CaptainStiggz]

Alright, that's reasonable. Just throwing {sync: true} into my code actually made a bunch of things crash. The workaround I'm using for now is

const state = derive({
  derived: (get) => {
     ...
     return {
       property1: ...,
       property2: ...,
     }
  }

I was also attempting derive state from 2 separate proxy objects. For that, I've just created a third proxy object, which I update with watch, which doesn't seem to have the same problems.

[dai-shi]

btw, here's an RFC related to this issue: #792

[dai-shi]

Transferred to: https://github.com/valtiojs/derive-valtio/issues/2