Skip to content

Current attributes in models become previous attributes too early#373

Closed
curvedmark wants to merge 13 commits intojashkenas:masterfrom
curvedmark:master
Closed

Current attributes in models become previous attributes too early#373
curvedmark wants to merge 13 commits intojashkenas:masterfrom
curvedmark:master

Conversation

@curvedmark
Copy link
Copy Markdown

The resetting of previous attributes seems to happen too early, i.e., as soon as all event listeners are called, current attributes become previous attributes, which really should happen the next time set() is called, otherwise the following code fails:

var Model = Backbone.Model.extend({
    foo: function() {
        this.set({key: 'val2'});
        this.trigger('fooevent');
    }
});

var model = new Model({key: 'val1'});

model.bind('fooevent', function() {
    console.log(model.previous('key'));
});

model.foo();

The output should be val1 and in backbone 0.3.3 it's val2

@jashkenas
Copy link
Copy Markdown
Owner

I'm afraid that the current behavior is how things are supposed to work. Previous attributes are only available during the course of a "change" event. Without this, the entire notion of hasChanged() makes no sense.

If you're looking for more robust undo-like functionality, I'd recommend keeping a true list of previous versions of the model.

@jashkenas jashkenas closed this May 27, 2011
@curvedmark
Copy link
Copy Markdown
Author

Thanks for the advice. But can you explain more on why hasChanged() makes no sense? I've run backbone through unit tests, all of them, especially the Model: change, hasChanged, changedAttributes, previous, previousAttributes test, didn't fail.

@jorgemanrubia
Copy link
Copy Markdown

+1 to this commit. It makes it easy to patch the Backbone.sync method for syncing only what has changed

@EdwardHinkle
Copy link
Copy Markdown

+1 to this commit

@jashkenas jashkenas reopened this Apr 11, 2014
@jashkenas
Copy link
Copy Markdown
Owner

@braddunbar, @tgriesser Any opinions on the semantics of this patch?

@tgriesser
Copy link
Copy Markdown
Collaborator

Well, the patch is really out of date (0.5.3) so I'd be curious to see what something like this looks like in the current form of the library. I'm not certain but I believe this might actually be the behavior in the latest? #2159

@jashkenas
Copy link
Copy Markdown
Owner

Right, sorry. Wasn't thinking.

@jashkenas jashkenas closed this Apr 11, 2014
@EdwardHinkle
Copy link
Copy Markdown

I realized after I +1'd it that it was pretty old code base. I'd be interested in delving more into it and submitting a patch, so I'll do that with a fork and submit it with the newer code base.

I tried to do this currently on the latest master branch and it still works the old way (previous gets overwritten by attributes during the end of set).

@harshal-dhumal
Copy link
Copy Markdown

The problem still persist if we have more than 1 model properties. Please see below example.


var Model = Backbone.Model;
var model = new Model({key: 'val1', key1: 'key1'});
model.on('change', function(){
    console.log(this.previous('key'));
    console.log(this.previous('key1'));
});
model.set({key: 'val2'});
model.set({key1: 'key2'});

Actual output
val1
key1
val2
key1

Expected output
val1
key1
val1
key1

Backbone version 1.1.2

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.

10 participants