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

Changed the model's set function so that the 'nested' variable isn't req... #2022

Merged
merged 1 commit into from
Dec 26, 2012

Conversation

adriaanlabusc
Copy link
Contributor

...uired

There used to be a line that read: this._pending = !!changes.length;
This caused this._pending to sometimes be set to false when model.set is called from within a change event. It would, for instance, happen during this test:

 test("final `change` event is fired when interim changes set an attribute without changing it's value", 1, function () {
    var model = new Backbone.Model({foo: 1, bar: 2});
    model.on('change:foo', function() {
      model.set({bar: 2});
      console.log('property changeevnt');
    });
    model.on('change', function() {
      ok(true);
    });
    model.set({foo: 10});
  });

During model.set({bar: 2});, this._pending gets set to false and therefore no change event would get triggered (without the use of nested). nested was used to trigger this event like so:

if (!this._pending && nested) this.trigger('change', this, options);

This commit changes the method so that nested is no longer required.

Note: the test is't in the test sweet, and all the current tests pass. Merry Christmas.

Signed-off-by: Adriaan Labuschagne adriaanlabuschagne@gmail.com

…required

Signed-off-by: Adriaan Labuschagne <adriaanlabuschagne@gmail.com>
@braddunbar
Copy link
Collaborator

Fantastic. Wish I would've come up with it myself. :)

@jashkenas @tgriesser Unless you guys have any reason not to merge this, I think it looks good.

tgriesser added a commit that referenced this pull request Dec 26, 2012
Changed the model's set function so that the 'nested' variable isn't req...
@tgriesser tgriesser merged commit 323d216 into jashkenas:master Dec 26, 2012
@tgriesser
Copy link
Collaborator

Looks good to me - thanks @adriaanlabusc!

@jashkenas
Copy link
Owner

Very, very nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants