caplet.js icon indicating copy to clipboard operation
caplet.js copied to clipboard

Changing data does not invoke onDataChange

Open nijikokun opened this issue 9 years ago • 7 comments

Perhaps I am missing some crucial information regarding the data object, it appears to be immutable, so does that mean I must create a snapshot every time I wish to modify it.

var UserModel = Caplet.createModelClass({
  initialize: function () {
    console.log('Initialized:', this.data)
  },

  onChange: function (data) {
    console.log('Saving Model Data', this.data)
  },

  setTokenData: function (token) {
    // here is where it happens, you have to clone it then set it
    var data = this.toData(this.data)
    data.token = token
    return this.set('data', data)
  }
})

The above will invoke onDataChange properly, however, this example will not:

this.set('data.token', token) 

Meaning now there are two states on the model, direct properties user.token and user.data.token which are no longer synced.

nijikokun avatar Jul 30 '15 00:07 nijikokun

Looked more into it and it has to do with watchable-object library, as it doesn't keep a snapshot, if you modify this.data that is modifying what I will call the source of truth and what is compared against meaning that it thinks there was no change.

A better solution is to make it immutable, or store a snapshot of data to compare against and update the snapshot only when onDataChange is fired.

nijikokun avatar Jul 30 '15 00:07 nijikokun

Currently doing something like this to get around this issue:

Caplet.Model.prototype.__proto__.onDataChange = function (data) {
  this.setProperties(this.fromData(data))
  this.state = this.toData(data)
}

Then doing:

  setTokenData: function (token) {
    this.state.token = token
    this.set('data', this.state)
  }

Now everything is propagated properly.

nijikokun avatar Jul 30 '15 01:07 nijikokun

Yep, the data property is the source of truth, and shouldn't be modified within the Model.

Got any ideas on how you'd like this API to be?

crcn avatar Jul 30 '15 01:07 crcn

Got any ideas on how you'd like this API to be?

I added a post of how I got around it, perhaps you have some thoughts on making a snapshot of data, and this.data is no longer the source of truth but rather a snapshot of the current state that data is

Yep, the data property is the source of truth, and shouldn't be modified within the Model.

How should it be updated, perhaps my approach is flawed, I can't do user.set('data.token') as it doesn't propagate the data change either since... data is source of truth, it looks like .fromData() and .toData() was made to get around this?


Another idea I had is to completely ditch .data and just use direct properties while relying solely on .onChange:

  onChange: function () {
    console.log('Saving Model Data', this.toData(this))
  },

  setTokenData: function (token) {
    this.set('token', token)
  }

nijikokun avatar Jul 30 '15 01:07 nijikokun

It seems ditching .data fares pretty well, using .data causes inconsistencies:

https://i.imgur.com/xvcgZXE.png

nijikokun avatar Jul 30 '15 01:07 nijikokun

The data property is still tremendously useful if you have a remote data source. I just wonder if it could either a) be moved to a mixin, or b) defined as a private property (.__data, instead of .data) since you should never really mutate the data property on a model explicitly. Here's a proper example of how I'm currently using the data prop in some production apps:

var mesh            = require("mesh");
var createMemoryBus = require("mesh-memory");
var caplet          = require("caplet");
var extend          = require("xtend/mutable");

var persistenceMixin = {

  /** 
   * load data on the model
   */

  load: function(onLoad) {
    return this.fetch({ name: "load", query: this.getSelfQuery() }, onLoad);
  },

  /** 
   * load data on the model
   */

  remove: function(onLoad) {
    return this.fetch({ name: "remove", query: this.getSelfQuery() }, onLoad);
  },

  /**
   */

  insert: function(onUpdate) {
    return this.fetch({ name: "insert" }, onUpdate);
  },

  /**
   */

  update: function(onUpdate) {
    return this.fetch({ name: "update", query: this.getSelfQuery() }, onUpdate);
  },

  /**
   */

  save: function(onSave) {
    return this.isNew() ? this.insert(onSave) : this.update(onSave);
  },

  /**
   */

  isNew: function() {
    return !this.data; // no data prop? No source of truth = new
  },

  /**
   */

  getSelfQuery: function() {
    var q = {};
    q[this.idProperty] = this[this.idProperty];
    return q;
  },

  /**
   */

  fetch: function(operation, onComplete) {
    if (!onComplete) onComplete = function() { };

    return this.bus(extend({
      name  : operationName,
      data  : this.data = this.toData() // freeze data for now
      model : this
    }, operation))
    .once("data", this.set.bind(this, "data"))
    .once("error", onComplete)
    .once("end", onComplete);
  }
}

var Person = caplet.createModelClass({
  idProperty: "name",
  mixins: [persistenceMixin],
  toData: function() {
    return {
      name: this.name,
      age: this.age
    };
  }
});

var bus = mesh.limit(1, createMemoryBus());

var person = Person({
  bus: bus,
  name: "Jake",
  age: 42
});

person.isNew(); // true
console.log(person.name); // Jake
person.save(); // bus({ name: "insert", data: { name: "Jake", age: 42 }})
person.age = 30;
person.save(); // bus({ name: "update", data: { name: "Jake", age: 30 }});

var person2 = Person({
  bus: bus,
  name: "Jake"
});

person2.load(function() {
  console.log(person2); // { name: "Jake", age: 42 }
}); // loads Jake

crcn avatar Aug 01 '15 17:08 crcn

My needs are less for a remote source and more of a localstorage situation, the only time we ever talk with remote is for loading. I suppose I could architect it this way, but that just introduces a lot of hassle, basically I would just be building my own model class at that point, so I wouldn't even need a library like this.

nijikokun avatar Aug 01 '15 23:08 nijikokun