AmpersandJS / ampersand-sync

Provides sync behavior for updating data from ampersand models and collections to the server.
http://ampersandjs.com
MIT License
20 stars 28 forks source link

dirty checking on patch? #35

Open yocontra opened 9 years ago

yocontra commented 9 years ago

it seems a little weird that you have to specify which fields go up when you save a model and want to patch only what has changed

Currently this will send the entire model:

model.save(null, {patch: true});

this will send the attributes that changed, but we have to manually specify the keys and values:

model.save({
  fieldOne: 123,
  fieldTwo: 123
}, {patch: true});

which is just as bad as not even using the model

request.patch('/api/users/' + model._id)
  .send({
    fieldOne: 123,
    fieldTwo: 123
  });
pgilad commented 9 years ago

The save method with patch:true imitates how Backbone behaves in this scenario.

There are 2 easy options to run dirty checking:

if (!this.model.hasChanged()) {
  return console.log('Nothing to update');
}
this.model.save(this.model.changedAttributes()), {patch:true));
//..
initialize: function() {
  this.originalModel = this.model.clone();
}
//...
onSave: function() {
 var changedAttrs = this.model.changedAttributes(this.originalModel.toJSON());
  if (!changedAttrs || !_.size(changedAttrs)) {
    return console.log('Nothing to update');
  }
  this.model.save(changedAttrs, {patch:true));
 //...
}

Another option is to integrate a plugin that tracks changes since the last save, something like this one: https://github.com/NYTimes/backbone.trackit

I tend to agree it's a lot of work to do a pretty common task

pgilad commented 9 years ago

This is something that needs to be resolved on ampersand-model. amp-sync is just a wrapper for xhr

yocontra commented 9 years ago

Is ampersand trying to improve where backbone fails or is it set on keeping failure parity?

naugtur commented 9 years ago

Well played :) It doesn't change the fact that its by no means the responsibility of ampersand-sync. ampersand-sync is a function. It doesn't have instances, it doesn't have state. Calculating the difference here would be a total breach of encapsulation.

We're talking about a reasonable feature here, but a feature for ampersand-model.

The usecase might not be that common to add substantial code to ampersand-model, so IMHO the best solution would be another small requireable module that'd add the functionality to models. Totally doable.

yocontra commented 9 years ago

Also @pgilad it doesn't seem like ampersand models have a .clone fn, just FYI