passing a `notifyInSync` flag to addComputed

[zcaudate]

I noticed that addComputed also uses subscribe.

https://github.com/pmndrs/valtio/blob/master/src/utils.ts#L192

Can a notifyInSync flag be passed into the function as well?

[dai-shi]

Would you try if it works? I'm afraid it can lead infinite loop in some cases. If the one tick delay is not acceptable in your use case, proxyWithComputed may work better. It's really annoying there are two options. I wished to provide a single solution, but they have pros/cons. And, notifyInSync=false is one of them. Even if it works, I'm not confident about how to properly explain in docs.

My gut feeling is in most cases derive would work and it would be more performant than addComputed. Would we re-consider #176? derive would work fine with notifyInSync=true.

[dai-shi]

(tbh, if derive would work well, it could cover most use cases of addComputed. And what's left can be done with proxyWithComputed. So, we could deprecate addComputed in favor of derive and proxyWithComputed. I believe it would be easier to understand and less confusions.)

[zcaudate]

Okay, I can see how the infinite loops might happen. Is it the versioning that causes it? My setup is not js based so it's kinda hard for me to test out non npm-based changes so I'll just make do for now.

I think watch and proxyWithComputed are quite similar so it wouldn't be a good idea to add derive - a third option. I actually really like addComputed because it's really easy to build an observable object step by step.

Looking at the source for proxyWithComputed, is it possible to implement addComputed with the method that proxyWithComputed uses?

[zcaudate]

I think proxyWithComputed is always synchronous right? because the computation happens on the call to the getter?

[dai-shi]

it wouldn't be a good idea to add derive - a third option.

okay... my idea was to drop addComputed to reduce options.

I actually really like addComputed

The reason I prefer derive to addComputed is addComputed and proxyWithComputed use proxy-compare internally for tracking property access. useSnapshot use proxy-compare too. Using proxy-compare too much wouldn't be ideal. (I mean if your concern is performance, it could behave contradictory. If your concern is code readability, no issue.)

On the other hand, watch and derive don't depend on proxy-compare. It's just a wrapper around subscribe. It's much more predictable and comes with less overhead (more predictable, could be better in performance).

Looking at the source for proxyWithComputed, is it possible to implement addComputed with the method that proxyWithComputed uses?

No, I spent huge amount of time trying this, and gave up. proxyWithComputed is a tricky implementation that happens to work because of the subtle behavior in vanilla.ts.

I think proxyWithComputed is always synchronous right? because the computation happens on the call to the getter?

Exactly. So, it behaves pretty natively. This getter behavior is very unique which works well by chance.

[zcaudate]

the reason I like addComputed is that it's more lower level than both watch and proxyWithComputed. You can see here that I'm using to it include an additional done field when constructing an execution graph tracker. done tracks the status field - which is a proxy. Use of addComputed is right at the bottom. If you think dropping addComputed would be the way moving forward, I'd still like a way to be able to implement what I currently have.

doesn't addComputed also use subscribe? what's the difference between this implementation and watch?

The code I'm talking about:

// js.proxy/graphGetRunnable
function graphGetRunnable(finish,tasks){
  return (tasks).filter(function ([k,{deps}]){
    return !(finish[k]) && (deps || []).every((sk) => (finish[sk]));
  });
}

// js.proxy/graphStartSingle
function graphStartSingle([k,{load}],finish,start){
  start[k] = Date.now();
  return (new Promise(function (resolve,reject){
    try{
      resolve((function (){
        return load();
      })());
    }
    catch(e){
      reject(e);
    }
  })).then(function (){
    return finish[k] = Date.now();
  });
}

// js.proxy/graphStartRunnable
function graphStartRunnable(tasks,finish,start){
  let runnable = graphGetRunnable(finish,tasks);
  (runnable).forEach(function (entry){
    graphStartSingle(entry,finish,start);
  });
}

// js.proxy/graphStartSubscribe
function graphStartSubscribe(tasks,finish,start){
  return ValtioCore.subscribe(finish,function (){
    graphStartRunnable(tasks,finish,start);
  },true);
}

// js.proxy/graphGetStatus
function graphGetStatus(tasks,finish,start){
  return Object.fromEntries((tasks).map(function ([k,e]){
    return [
      k,
      (finish[k]) ? "done" : ((start[k]) ? "running" : "waiting")
    ];
  }));
}

// js.proxy/graphCreateStatus
function graphCreateStatus(tasks,finish,start){
  let status = ValtioCore.proxy(graphGetStatus(tasks,finish,start));
  ValtioCore.subscribe(finish,function (){
    Object.assign(status,graphGetStatus(tasks,finish,start));
  },true);
  ValtioCore.subscribe(start,function (){
    Object.assign(status,graphGetStatus(tasks,finish,start));
  },true);
  return status;
}

// js.proxy/attachGraphFn
function attachGraphFn(pobj,tasks){
  let finish = ValtioCore.proxy({});
  let start = ValtioCore.proxy({});
  let resetFn = function (){
    pobj.stop();
    js.delKeys(finish);
    js.delKeys(start);
    graphDepsUnload(tasks);
    pobj["stop"] = graphStartSubscribe(tasks,finish,start);
  };
  let loadFn = function (name){
    let entry = ((tasks).filter(([k]) => k == name))[0];
    if(entry){
      graphStartSingle(entry,finish,start);
    };
  };
  let startFn = function (){
    graphStartRunnable(tasks,finish,start);
  };
  let status = graphCreateStatus(tasks,finish,start);
  Object.assign(pobj,{
    "load":loadFn,
    "reset":resetFn,
    "check":() => graphDepsCheck(tasks),
    "start":startFn,
    "tasks":tasks,
    "stop":graphStartSubscribe(tasks,finish,start),
    "status":status,
    "timing":ValtioCore.ref({"start":start,"finish":finish}),
    "deps":() => graphDepsOrder(tasks)
  });
  ValtioUtils.addComputed(pobj,{
    "done":function (pobj){
        return (Object.values(pobj.status)).every((v) => v == "done");
      }
  });
  return pobj;
}

The usage for the attachGraphFn is something like this (as an initial boot loading process). future-delayed is a shortcut for a promise that resolves after [ms] time. Graph.start() will execute all tasks that are free of dependents.

(def.js ^{:dev true} Graph
 (attachGraphFn (proxy {})
  [["buy-milk"   {:label "Buy Milk"
                  :load (:> [] (js/future-delayed [300]))}]
   ["buy-sugar"  {:label "Buy Suger"
                  :load (:> [] (js/future-delayed [500]))}]
   ["buy-eggs"   {:label "Buy Eggs"
                  :load (:> [] (js/future-delayed [500]))}]
   ["cook-eggs"  {:label  "Cook Eggs"
                  :load (:> [] (js/future-delayed [300]))
                  :deps ["buy-eggs"]}]
   ["cook-milk"  {:label  "Cook Milk"
                  :load (:> [] (js/future-delayed [300]))
                  :deps ["buy-milk"
                         "buy-sugar"]}]
   ["serve-meal" {:label  "Serve Meal" 
                  :load (:> [] (js/future-delayed [300]))
                  :deps ["cook-eggs"
                         "cook-milk"]}]]))

[zcaudate]

I realised that the last code block was in lisp (which is terser but maybe harder to understand). The expanded javascript is:

// js.proxy/createGraph
function createGraph(tasks){
  let pobj = ValtioCore.proxy({});
  attachGraphFn(pobj,tasks);
  return pobj;
}

Graph = s.createGraph([
  [
  "buy-milk",
  {
  "label":"Buy Milk",
  "load":() => new Promise(function (resolve,reject){
    setTimeout(function (){
      try{
        resolve((function (){

        })());
      }
      catch(e){
        reject(e);
      }
    },300);
  })
}
],
  [
  "buy-sugar",
  {
  "label":"Buy Suger",
  "load":() => new Promise(function (resolve,reject){
    setTimeout(function (){
      try{
        resolve((function (){

        })());
      }
      catch(e){
        reject(e);
      }
    },500);
  })
}
],
  [
  "buy-eggs",
  {
  "label":"Buy Eggs",
  "load":() => new Promise(function (resolve,reject){
    setTimeout(function (){
      try{
        resolve((function (){

        })());
      }
      catch(e){
        reject(e);
      }
    },500);
  })
}
],
  [
  "cook-eggs",
  {
  "label":"Cook Eggs",
  "load":() => new Promise(function (resolve,reject){
    setTimeout(function (){
      try{
        resolve((function (){

        })());
      }
      catch(e){
        reject(e);
      }
    },300);
  }),
  "deps":["buy-eggs"]
}
],
  [
  "cook-milk",
  {
  "label":"Cook Milk",
  "load":() => new Promise(function (resolve,reject){
    setTimeout(function (){
      try{
        resolve((function (){

        })());
      }
      catch(e){
        reject(e);
      }
    },300);
  }),
  "deps":["buy-milk","buy-sugar"]
}
],
  [
  "serve-meal",
  {
  "label":"Serve Meal",
  "load":() => new Promise(function (resolve,reject){
    setTimeout(function (){
      try{
        resolve((function (){

        })());
      }
      catch(e){
        reject(e);
      }
    },300);
  }),
  "deps":["cook-eggs","cook-milk"]
}
]
])

[dai-shi]

doesn't addComputed also use subscribe? what's the difference between this implementation and watch?

addComputed uses proxy-compare which you don't need in your use case, because property access tracking is not of your interest. It's just unnecessary overhead in such use cases.

If you think dropping addComputed would be the way moving forward, I'd still like a way to be able to implement what I currently have.

There's a big difference between addComputed and derive. The former modify existing proxy, and the latter returns a new proxy.

So, your example would become like this.

function attachGraphFn(pobj,tasks){
  let finish = ValtioCore.proxy({});
  let start = ValtioCore.proxy({});
  let resetFn = function (){
    pobj.stop();
    js.delKeys(finish);
    js.delKeys(start);
    graphDepsUnload(tasks);
    pobj["stop"] = graphStartSubscribe(tasks,finish,start);
  };
  let loadFn = function (name){
    let entry = ((tasks).filter(([k]) => k == name))[0];
    if(entry){
      graphStartSingle(entry,finish,start);
    };
  };
  let startFn = function (){
    graphStartRunnable(tasks,finish,start);
  };
  let status = graphCreateStatus(tasks,finish,start);
  let derived = ValtioUtils.derive((get) => (Object.values(get(status))).every((v) => v == "done");
  Object.assign(pobj,{
    "load":loadFn,
    "reset":resetFn,
    "check":() => graphDepsCheck(tasks),
    "start":startFn,
    "tasks":tasks,
    "stop":graphStartSubscribe(tasks,finish,start),
    "status":status,
    "timing":ValtioCore.ref({"start":start,"finish":finish}),
    "deps":() => graphDepsOrder(tasks),
    "derived": derived
  });
  return pobj;
}

We might be able to make derive to attach properties to an existing field.

Yeah, I would like to go with this road. Let me reopen #176.

[zcaudate]

Okay, I see what you’re saying. In the case where derive attaches to a proxy, I’d like for the semantic to be something like this:

// js.proxy/attachGraphFn function attachGraphFn(pobj,tasks){ ...
Object.assign(pobj,{ "load":loadFn, "reset":resetFn, "check":() => graphDepsCheck(tasks), "start":startFn, "tasks":tasks, "stop":graphStartSubscribe(tasks,finish,start), "status":status, "timing":ValtioCore.ref({"start":start,"finish":finish}), "deps":() => graphDepsOrder(tasks) }); // derive(function, notifyInSync?, proxy?) let derived = ValtioUtils.derive((get) => (Object.values(get(status))).every((v) => v == “done”, true, pobj); return pobj; }

[dai-shi]

Actually, we need a key for function. And, I think we prefer { sync: true } like useSnapshot for util apis. (subscribe is more considered as a primitive api.)

const derived = derive({
  done: (get) => Object.values(get(status)).every((v) => v === 'done'),
}, {
  sync: true,
  proxy: pobj,
})

// In this case derived === pobj

[zcaudate]

Yep. that works for me!