Playing with transparent Proxy wrapper for s-js...

[andyrj]

I wanted to ask two questions, coming from a little mobx experience and having attempted to write my own observable state lib I was wondering if making a Proxy wrapper had been considered so that use of the s-js lib begins to look like POJO?

const S = require("s-js");
const SArray = require("s-array");

function isData(fn) {
  return fn.toString().startsWith("function data(value)");
}

function isComputation(fn) {
  return fn.toString().startsWith("function computation()");
}

// maybe this should be called s-pojo or something
function Store(state, actions) {
  const store = {};
  const proxy = new Proxy(store, {
    get: function(target, name) {
      if (name in target) {
        if (
          typeof target[name] === "function" &&
          (isData(target[name]) || isComputation(target[name]))
        ) {
          return target[name]();
        } else {
          return target[name];
        }
      } else {
        if (name in actions) {
          return actions[name];
        } else {
          return undefined;
        }
      }
    },
    set: function(target, name, value) {
      if (name in target) {
        if (typeof target[name] === "function") {
          if (isData(target[name])) {
            target[name](value);
          } else if (isComputation(target[name])) {
            return false;
          }
        } else {
          target[name] = value;
        }
        return true;
      } else {
        if (Array.isArray(value)) {
          target[name] = SArray(value);
        } else if (typeof value === "function") {
          let fn = value.bind(proxy);
          target[name] = S(fn);
        } else if (typeof value === "object") {
          // write logic here to recursively create new proxy for this obect?
          return false;
        } else {
          target[name] = S.data(value);
        }
        return true;
      }
    }
  });

  // attach data and computations to proxy...
  Object.keys(state).forEach(key => {
    proxy[key] = state[key];
  });

  return proxy;
}

// I prefer some kind of wrapper so that I can just pass POJO & functions...
const store = Store(
  {
    counter: 0,
    first: "Andy",
    last: "Johnson",
    /*nested: {
      foo: "BAR"
    },*/
    fullName: function() {
      return `${this.first} ${this.last}`;
    },
    fullCount: function() {
      return `${this.fullName}: ${this.counter}`;
    }
  },
  {
    updateData(firstName, lastName, count) {
      S.freeze(() => {
        this.counter = count;
        this.first = firstName;
        this.last = lastName;
      });
    }
  }
);

//  basically mobx autorun
S.root(() => {
  S(() => {
    console.log(store.fullName);
  });
  S(() => {
    console.log(store.fullCount);
  });
});

store.updateData("Jon", "Doe", 10);

I love the library I just dislike the set/get via function calls personally.

Is there anyway we could make the warning: "computations created without a root or parent will never be disposed" optional? I understand what it's complaining about since my computations are outside of the S.root thunk and thus won't be disposed, but I couldn't really find a better way to structure this to emulate what essentially is mobx lol (other than handling the nested object case...).

First day messing with the library so maybe I have overlooked how to avoid that error message for this kind of use case or maybe I'm completely off in left field.

[andyrj]

One more question, the above code output seems a bit odd as far as order of execution inside of a S.root thunk... I get...

Andy Johnson
Andy Johnson: 0
Jon Doe: 10
Jon Doe

I expected to get

Andy Johnson
Andy Johnson: 0
Jon Doe
Jon Doe: 10

not a huge deal and it may be that way on purpose for nested computation wrapped jsx components in surplus, just curious if this is expected?

[ismail-codar]

For question one: While you are building store proxy[key] = state[key]; runs proxy set function but S.root(() => { is not constructed this time and S.js warns: "computations created without a root or parent will never be disposed" You should find a solution to this.. Object.defineProperty approach can be an alternative rather then Proxy.. Like this:

function SPojo(obj) {
    Object.keys(obj).forEach(key => {
        if (typeof obj[key] !== "function") {
            //(function () {
                let sData = S.data(obj[key]);
                Object.defineProperty(obj, key, {
                    enumerable: true,
                    get: sData,
                    set: sData
                })
           // })()
        }
    });
    return obj
}

const user = SPojo(
    {
        counter: 0,
        first: "Andy",
        last: "Johnson",
        nested: SPojo({
            foo: "BAR"
        }),
        fullName: function () {
            return `${this.first} ${this.last}`;
        },
        fullCount: function () {
            return `${this.fullName()}: ${this.counter}`;
        }
    }
);

[ismail-codar]

But seems to be a problem with related the second question. Here is simplified simulation for problem:

const S = require("s-js");

var user = {
    counter: S.data(0),
    first: S.data("Andy"),
    last: S.data("Johnson"),
    fullName: function () {
        return `${this.first()} ${this.last()}`;
    },
    fullCount: function () {
        return `${this.fullName()}: ${this.counter()}`;
    }
};

S.root(() => {
    S(() => {
        console.log(user.fullName());
    });
    S(() => {
        console.log(user.fullCount());
    });
});

/* EXPECTED
Andy Johnson
Andy Johnson: 0
Jon Doe
Jon Doe: 10
*/
S.freeze(() => {
    user.counter(10);
    user.first("Jon");
    user.last("Doe");
});
/*BUT PRINTS
Andy Johnson
Andy Johnson: 0
Jon Doe: 10
Jon Doe
*/

@adamhaile can help.

[adamhaile]

A lot of really cool stuff here. I'm against a deadline at present but will try to respond longer over the weekend.

On the last point, if you have expectations about the order two computations will run, then that's a dependency between them, and you need to be explicit about it:

S.root(() => {
  const logName = S(() => {
    console.log(store.fullName);
  });
  S(() => {
    S.sample(logName);
    console.log(store.fullCount);
  });
});

Computations produce both a value and side effects, like logging to console. If you expect fullCount to be logged after fullName, that's a dependency between the two computations' side effects. Dependency on a computation's value happens organically, when we dereference it to get its current value. Dependencies on side effects often require us to be explicit, like here.

Sampling is enough to ensure execution order. Alternately, you could dereference it like logName(), but that would mean the second computation would always run when the first did. That happens to be the case here, since fullCount references fullName, but might not be in other scenarios.

[andyrj]

@adamhaile Thank you the order of computations makes sense now.

@ismail-codar I am partial to the flexibility of Proxy, I know that get/set could be written with defineProperty as you demonstrated. But with defineProperty you can't overload deleteProperty, has, and others like you can with Proxy. Some of which I plan to use before this little pojo experiment is complete. And for me supporting ie and opera mini isn't a concern.

[andyrj]

Updated code... made a few changes to Proxy usage and fixed the errors about not disposing of the computations, I think this is shaping up to be a nice little api for semi-transparently using s-js. ~I need to figure out why my use of S.freeze() for my actions isn't working though...~

//import S from "s-js";
const S = require("s-js");

function isData(fn) {
  return fn.toString().startsWith("function data(value)");
}

function isComputation(fn) {
  return fn.toString().startsWith("function computation()");
}

const arrayMutators = [
  "splice",
  "push",
  "unshift",
  "pop",
  "shift",
  "copyWithin",
  "reverse"
];

function Store(state, actions) {
  const store = function() {};
  const stateKeys = [];
  const disposers = {};
  const proxy = new Proxy(store, {
    get: function(target, name) {
      if (name in target) {
        if (typeof target[name] === "function") {
          if (isData(target[name])) {
            let val = target[name]();
            if (Array.isArray(val)) {
              arrayMutators.forEach(key => {
                val[key] = function() {
                  const res = Array.prototype[key].apply(this, arguments);
                  target[name](val);
                  return res;
                };
              });
              return val;
            } else {
              return val;
            }
          } else if (isComputation(target[name])) {
            return target[name]();
          } else {
            return target[name];
          }
        }
      } else {
        if (name in actions) {
          return actions[name];
        } else {
          return undefined;
        }
      }
    },
    set: function(target, name, value) {
      if (name in target) {
        if (typeof target[name] === "function") {
          if (isData(target[name])) {
            target[name](value);
          } else if (isComputation(target[name])) {
            return false;
          }
        } else {
          target[name] = value;
        }
        return true;
      } else {
        // breaking from pojo behavior, changes made through apply
        return false;
      }
    },
    has: function(target, name) {
      // we only want for..in loops to operate on state, not actions...
      if (stateKeys.indexOf(name) > -1) {
        return true;
      } else {
        return false;
      }
    },
    ownKeys: function(target) {
      return Reflect.ownKeys(target).filter(k => {
        return (
          stateKeys.indexOf(k) > -1 ||
          k === "caller" ||
          k === "prototype" ||
          k === "arguments"
        );
      });
    },
    /* eslint-disable complexity */
    apply: function(target, context, args) {
      // function to modify the observable...
      //store("data", "key", value, {observed=false});
      //store("action", "key", fn);
      //store("computed", "key", fn);
      //store("store", "key", {state: {}, actions: {}});
      //store("dispose")
      const t = args[0];
      const key = args[1];
      const val = args[2];
      const opts = args[3];
      switch (t) {
        case "data":
          stateKeys.push(key);
          if (opts && opts.observed === false) {
            target[key] = val;
          } else {
            // default to observable data...
            target[key] = S.data(val);
          }
          break;
        case "action":
          const fn = val;
          target[key] = function() {
            const actionArgs = arguments;
            S.freeze(() => {
              fn.apply(proxy, actionArgs);
            });
          };
          break;
        case "computed":
          stateKeys.push(key);
          const comp = val.bind(proxy);
          S.root(dispose => {
            target[key] = S(comp);
            disposers[key] = dispose;
          });
          break;
        case "store":
          const s = val.state || {};
          const a = val.actions || {};
          target[key] = Store(s, a);
          disposers[key] = () => target[key]("dispose");
          break;
        case "dispose":
          disposers.forEach(key => disposers[key]());
          target.forEach(key => delete target[key]);
          break;
        default:
          throw new RangeError(
            "type must be one of the following: data, dispose, action, computed, store"
          );
      }
      /* eslint-enable complexity */
    },
    deleteProperty: function(target, name) {
      // handle removing observer stuff for this key if needed..
      if (name in target) {
        if (name in disposers) {
          disposers[name]();
        }
        delete target[name];
        return true;
      } else {
        return false;
      }
    }
  });

  // attach data and computations to proxy...
  Object.keys(state).forEach(key => {
    //proxy[key] = state[key];
    const val = state[key];
    const t = typeof val;
    switch (t) {
      case "function":
        // computed
        proxy("computed", key, val);
        break;
      case "object":
        if (!Array.isArray(state[key])) {
          // nested store
          proxy("store", key, { state: val });
          break;
        }
      // fallthrough to add array as data...
      default:
        // add rest as data...
        proxy("data", key, val);
        break;
    }
  });

  Object.keys(actions).forEach(key => {
    proxy("action", key, actions[key]);
  });

  return proxy;
}

const store = Store(
  {
    counter: 0,
    first: "Andy",
    last: "Johnson",
    /*nested: {
      foo: "BAR"
    },*/
    fullName() {
      return `${this.first} ${this.last}`;
    },
    fullCount() {
      return `${this.fullName}: ${this.counter}`;
    }
  },
  {
    updateData(firstName, lastName, count) {
      this.counter = count;
      this.first = firstName;
      this.last = lastName;
    }
  }
);

S.root(() => {
  S(() => {
    console.log(store.fullName);
  });
  S(() => {
    console.log(store.fullCount);
  });
});

// example usage
store.updateData("Jon", "Doe", 10);
/* same as action used above...
S.freeze(() => {
  store.first = "Jon";
  store.last = "Doe";
  store.count = 10;
});
*/

// will log all keys excluding actions
for (let v in store) {
  console.log(`${v}: ${v in store}`);
}

I realize this is probably out of scope for s-js itself, just thought I'd share the experiment... ~I also thought maybe one of you might see what's wrong with my action logic.~ Fixed the action code, arrow function was messing with my arguments context...

edits: sorry about all the edits, fixed for..in loops and some other errors... didn't want for loops iterating over action keys as that doesn't really make sense...

[andyrj]

if anyone is interested in the above I made a npm package and github repo for the little experiment, for now. Calling it Proxied Observable State Tree post-js, gonna add snapshot/patch/restore to it and show it hooked up to redux-devtools :smile:

[adamhaile]

Hi Andy -- Sorry for the slow reply, it ended up being a busy weekend.

I played around with doing something like this but never got to the point I had something I thought was ready to share. There are obvious advantages to preserving a natural obj.foo get and obj.foo = val set behavior, especially for people new to S. For reference, these were the open questions in my mind:

1. Design. At the time, MobX's observablization was deep and infectious, and that made me a bit uneasy. I worried that the infectious behavior would lead to surprises like:

   var todo = { title: "foo", complete: false };
   store.todos[0] = todo;
   store.todos[0] === todo; // FALSE!

   As for the deep behavior, I also worked some years ago on an app that did deep observablization with knockout, and it led to increasing inertia as the app grew. Models that were just observablized JSON meant they were very weak (no methods, no computed fields, etc). Lots of functionality that should have been in the model got copied across controllers and view models. Midway in the app's life we had to tear out the observablize utility and replace it with real constructors, each responsible for its own shallow observablization. So the utility I was exploring was something like MobX's extendShallowObservable(this, { ... data ... }), but overall I wasn't sure what the right way forward was. You've got much more experience with MobX than me, so I'd be curious to hear about how the MobX community thinks about these issues now.

2. Performance. I was using defineProperty(), and at the time, I benched it as being about 3x slower than native S. This was a couple years ago, so the browsers may be better at optimizing defineProperty() now. I would expect a Proxy-based implementation to have a considerable performance loss, not just because the VMs might not optimize Proxy yet, but also because any time you use strings as identifiers, you lose most of the VMs' optimizations around property access and fall back to hashtable access. If it's useful, the current benchmarking script I use during S development is in the git repo. Pull and run npm run bench and you'll see it.

3. Less powerful. For my own code, I find it very powerful to have first-class signals. I can pass signals to functions, attach them to other objects, build higher-ordered signals, etc. Embedding the signals behind properties means you favor passing whole objects instead. One thing I'd explored in my POJO utility was an ability to access the backing signals.

Please don't read this as saying what you're doing is a bad idea -- not at all! Like I say, I think POJO-like access could be really useful, just still playing with the right direction.

A couple more thoughts on the specific code:

1. Have you looked at s-array? It proxies array mutators and does it in a way that preserves synchronous immutability. With your code, arrays are modified immediately:

   
   const store = Store({ foos: [] });
   S.freeze(() => {
   store.foos(); // returns []
   store.foos.push(1);
   store.foos(); // returns [1], signal mutated during freeze
   });

const sa = SArray([]); S.freeze(() => { sa(); // returns [] sa.push(1); sa(); // still returns [] }); sa(); // now returns [1]

2. Converting all functions to parentless computations means you lose S's ability to manage their lifecycle for you and end up having to do manual resource management.  If you miss calling `object("dispose")` somewhere in your code, you have both a memory and a performance leak.  I would consider *not* wrapping passed-in functions with S().  Those properties will still be reactive -- calling code will "see through" the getter to the referenced signals.  They'll re-evaluate every time they're referenced, but that's not necessarily a bad thing.  In most cases, like here, computed properties are pretty short functions, and their runtime is often similar to the book-keeping cost of calling a computation.  And if they do non-trivial work, I'd want greater visibility into that fact than an automatic conversion like this provides.

Best
-- Adam

[andyrj]

Thanks for the feedback! First, no need to apologize ever for giving me code feedback. I also wouldn't call myself a mobx pro, just think it has some interesting properties that seem to be a subset of your s-js, and mobx-state-tree, which I am aiming to replicate adds hooks to utilize things like redux-devtools. I really am hoping I can make escape hatches so that the default limiting behavior you are talking about isn't permanently restrictive or otherwise detrimental. But if my lib is not used, it would be fine, I learned about s-js, proxy, and defineProperty which is more important to me anyways.

So, as for the infectious observable, I left an escape hatch for unobserved data, maybe I could/should extend that. I need to think about that some more.

I obviously made a naive mistake with my array wrappers I will have to fix. I will reference SArray for help there. I initially was going to just use SArray and maybe that is what would be best.

After writing tests I was looking at it and the only thing I can't recreate without Proxy, is the deleteProperty trap, so for performance I am looking to convert to defineProperty now, perhaps even build a prototype factory, to help with defineProperty's overhead. Just started digging into that last night. I will have to check out your bench code and see if I can't make an adaptation of it to identify how much overhead my wrapper is adding(which I am sure is significant at the moment). My initial code was just to make it work, I figure then I can measure and optimize as needed.

As for memory leaks, I am fairly certain I kept all the handles I generated for all the dispose calls, and exercise them on key delete and store dispose. I get how that could be a concern, I could write some tests with a headless chrome instance to exercise the library and inspect the heap to verify that it is cleaning up after itself. Not sure what else I have off the top of my head on that, will have to think about it some more.

On the automatic conversion point, that is understandable, I am sure this is not how you expected to use your library but I greatly appreciate your insights, helps me tremendously.

I will think and tinker with my library and see if I can't address the concerns you raised. For now I think I will close this issue, and I will reach out and ping you again after I have made improvements and benchmarks.

Thank you again for open sourcing s-js, it is really amazing.

[andyrj]

One more thing lol. I did change quite a bit from the code in this issue to the code in my repo. The surprise inequality should be less jarring in my repo as the set trap is no longer used to define new keys on the store. Instead I decided to make store a function with properties, so adding new observable data to store after init is like, store("data", "key", value);

[ismail-codar]

I've been thinking a lot about this before. Here is a similar topic with this topic. https://github.com/paldepind/flyd/issues/142#issuecomment-294183297 I remember to first time I came to surpus from this page.

For example: https://github.com/ccorcos/reactive-magic project switched to using obj.set(value) and value = obj.get() style at later time.

Using natural obj.foo get and obj.foo = val style is initially quite liked. But when you see problems (similar to adamhaile is saying), it does not seem worth it.

[andyrj]

@ismail-codar I will read those links, thank you for your feedback as well, it is greatly appreciated.

I hope I haven't wasted either of your time with my experiment.

edit: also I really like the pattern used by s-js with closures instead of boxing values within objects as I believe mobx does/or at least did.

[andyrj]

@adamhaile https://codepen.io/andyrj/pen/NaxXgX my implementation doesn't have that infectious issue with equality you were referencing at least not when I tried to replicate it in a codepen. I'm only really looking at possibly "infecting" nested objects, but not objects appended to arrays, weird thing is even when I "infect" an object that equality issue you reference doesn't come up I believe due to using Proxy... I also fixed how I was wrapping array mutators and made test case to verify that the case you pointed out is corrected.

[andyrj]

so I believe I have added snapshots with structural sharing and json-patch output to my little experiment...

https://github.com/andyrj/post-js/blob/snapshots/src/store.js#L325

gonna add tests for it so I keep my 100% coverage, but should be able to hook up redux-devtools extension to this soon...