New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dirty checking on patch? #35

Open
contra opened this Issue Feb 24, 2015 · 5 comments

Comments

Projects
None yet
4 participants
@contra

contra commented Feb 24, 2015

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

This comment has been minimized.

Show comment
Hide comment
@pgilad

pgilad Apr 4, 2015

Member

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

There are 2 easy options to run dirty checking:

  • model.changedAttributes - changes since the last set. My experience is that this doesn't work with forms that run multiple sets, as only the last change since set is saved.
if (!this.model.hasChanged()) {
  return console.log('Nothing to update');
}
this.model.save(this.model.changedAttributes()), {patch:true));
  • Have a copy of the original model before any changes, and run a diff:
//..
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

Member

pgilad commented Apr 4, 2015

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

There are 2 easy options to run dirty checking:

  • model.changedAttributes - changes since the last set. My experience is that this doesn't work with forms that run multiple sets, as only the last change since set is saved.
if (!this.model.hasChanged()) {
  return console.log('Nothing to update');
}
this.model.save(this.model.changedAttributes()), {patch:true));
  • Have a copy of the original model before any changes, and run a diff:
//..
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

This comment has been minimized.

Show comment
Hide comment
@pgilad

pgilad Aug 8, 2015

Member

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

Member

pgilad commented Aug 8, 2015

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

@contra

This comment has been minimized.

Show comment
Hide comment
@contra

contra Aug 9, 2015

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

contra commented Aug 9, 2015

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

@naugtur

This comment has been minimized.

Show comment
Hide comment
@naugtur

naugtur Aug 10, 2015

Member

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.

Member

naugtur commented Aug 10, 2015

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.

@contra

This comment has been minimized.

Show comment
Hide comment
@contra

contra Sep 2, 2015

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

contra commented Sep 2, 2015

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment