backbone icon indicating copy to clipboard operation
backbone copied to clipboard

Implement reset option for model.set

Open blikblum opened this issue 8 years ago • 7 comments

Implements the reset option for model.set. Fixes #3253 #4098

When reset flag is passed as a truthy value to model.set, the properties that exist in model.attributes but not in the attributes being set will be deleted triggering the appropriate change event, effectively resetting the model state. If reset flag is passed to model.fetch and the request succeeds the model state will be reset with the values returned by the ajax call.

This feature is necessary because the primitives provided by Backbone now does not allow to get such behavior without shortcomings.

model.clear().set(newattrs) will fire change events twice

model.clear({silent:true}).set(newattrs) will not fire change event for the unset properties

model.clear().fetch() suffers from the above issues plus if request fails, will end with an empty model

Also, in all the situations above, passing validate flag and validation failing, will end with an empty model

Important to note that the changes introduced by this PR are narrow and backward compatible.

Glad to iterate the changes if deems appropriate.

blikblum avatar Nov 20 '16 19:11 blikblum

This has been brought up and turned down a few times in the past. What's wrong with model.clear(); model.set(model.defaults());?

akre54 avatar Nov 29 '16 17:11 akre54

I'm on board with it, and it's been requested multiple times. We'll probably have a weird problem with Collection#fetch's reset option, though.

jridgewell avatar Nov 29 '16 18:11 jridgewell

AFAIK is the first PR, all other were issues.

Clarification: it does not introduce a new method reset. It adds an option. It could be used for arbritary values other than defaults. This is how is supposed to be used: model.set({a: 1}, {reset: true})

It does not change behavior of current code

What's wrong with model.clear(); model.set(model.defaults());?

  1. It will fire change event twice for attributes that are updated and will fire change once for attributes that keeps the same value

In a model with attributes {a: 1, b: 2, c: 3}

model.clear(); model.set({a: 1, b: 3}); 'change:a' is fired once, 'change:b' is fired twice, 'change:c' is fired once

model.set({a: 1, b: 3}, {reset: true}); 'change:a' is not fired, 'change:b' is fired once, 'change:c' is fired once

  1. If model.set fails due to validation we would end with an empty model

blikblum avatar Nov 29 '16 21:11 blikblum

We'll probably have a weird problem with Collection#fetch's reset option, though.

Can you elaborate the problem?

It does not change behavior of existing code. The change is restricted to set method and does not touch in fetch code at all.

It will work for fetch also because the reset option is propagated to model.set that is only called inside fetch after a sucessfull request

blikblum avatar Nov 29 '16 21:11 blikblum

I like the reset option as well but I don't think using delete on each key is the way to go.

Would just replacing the attributes hash with a new object be more efficient?

Edit: as a reference, see an implementation I'm using.

emileber avatar Jan 04 '17 19:01 emileber

How about using the name replace instead of reset?

reset has a different meaning when fetching a collection. If you wanted to re-fetch a collection with resetting the models using your options, fetch({merge: true, reset: true}) would reset the collection instead of merging. fetch({merge: true, replace: true}) would work as you expected.

prantlf avatar Jan 07 '18 19:01 prantlf

I think this PR is still valid.

The suggested workaround model.clear(); model.set(model.defaults());, by @akre54, doesn't work correctly because of either duplicate or non-existence change events. Those cases are described here: https://github.com/jashkenas/backbone/issues/3253#issuecomment-135461345

And if this really is a duplicate of some other item, I think that other item should be referenced.

ksurakka avatar Feb 21 '18 11:02 ksurakka