Skip to content
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

Implement reset option for model.set #4101

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

blikblum
Copy link

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.

@akre54
Copy link
Collaborator

akre54 commented Nov 29, 2016

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

@jridgewell
Copy link
Collaborator

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.

@blikblum
Copy link
Author

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
Copy link
Author

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

@emileber
Copy link
Contributor

emileber commented Jan 4, 2017

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.

@prantlf
Copy link

prantlf commented Jan 7, 2018

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.

@blikblum blikblum force-pushed the implement-model-reset branch from 1d573ae to 2a33522 Compare January 26, 2018 23:22
@ksurakka
Copy link

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: #3253 (comment)

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

@blikblum blikblum force-pushed the implement-model-reset branch from 2a33522 to 3a9621b Compare May 24, 2018 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a "replace" option to Backbone.Model.set
8 participants