From 0d5d9a8e5ece14b7df1c467be4d78a544c7821c6 Mon Sep 17 00:00:00 2001 From: fcamblor Date: Wed, 6 May 2015 19:12:07 +0200 Subject: [PATCH 1/2] Provided test allowing to verify we don't put any watchers on some nested Backbone.Model when we use Backbone.NestedModel --- test/nested-model.js | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/test/nested-model.js b/test/nested-model.js index baed677..c7b32ba 100644 --- a/test/nested-model.js +++ b/test/nested-model.js @@ -1117,4 +1117,40 @@ $(document).ready(function() { equal(model, doc); }); + test("#walkPath() should not observe Backbone.Models properties, you should observe this by yourself directly by binding watchers on submodels", function(){ + var nonAttributeChange = sinon.spy(); + var changeFromRootModel = sinon.spy(); + var changeFromSubModel = sinon.spy(); + + var model = new Backbone.NestedModel({ + submodel: new Backbone.Model({ foo: "bar" }) + }); + + // submodel.cid is amongst the Backbone.Model fields which should not be observable + model.bind('change:submodel.cid', nonAttributeChange); + + // By defining a new Backbone.Model instance, we're implicitely changing submodel.cid field + model.set("submodel", new Backbone.Model(model.get("submodel").toJSON())); + sinon.assert.notCalled(nonAttributeChange); + + // We should avoid setting sub backbone models watchers directly through model props + // because it may lead to lots of observers set up on Backbone.Model props + // particularly _events props which may contain huge objects + model.bind("change:submodel.attributes", changeFromRootModel); + + // Trying to update submodel attributes as well... but this should not be allowed either + // because here, we won't trigger any submodel's event (only model's event) + // We'll see a better way to handle this below... + model.set("submodel.attributes.foo", "bar"); + sinon.assert.calledOnce(changeFromRootModel); + + + // We should rather consider binding the change event directly on the submodel + model.get("submodel").bind("change:foo", changeFromSubModel); + + // This will be far better and will trigger an update this time + model.get("submodel").set("foo", "quix"); + sinon.assert.calledOnce(changeFromSubModel); + }); + }); From 1804ae6e2093da388ef773949728710afe8ecaa9 Mon Sep 17 00:00:00 2001 From: fcamblor Date: Thu, 7 May 2015 01:44:54 +0200 Subject: [PATCH 2/2] Not checking changes on NestedModel sub Backbone.Model/Collection as it will generate lots of useless observers, particularly on Backbone.Model._events which may contain huge event binding data --- backbone-nested.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/backbone-nested.js b/backbone-nested.js index 1d4d657..7b8d84a 100644 --- a/backbone-nested.js +++ b/backbone-nested.js @@ -267,8 +267,13 @@ if (!opts.silent && _.isObject(newValue) && isNewValue){ var visited = []; var checkChanges = function(obj, prefix) { - // Don't choke on circular references - if(_.indexOf(visited, obj) > -1) { + // Not checking changes on sub-Backbone.Models/Collection props + // in order to avoid to put observers on potential heavy objects + // (typically, Backbone.Models have _events properties which may have lots of things inside it, and + // we should not put useless observers on it) + if(obj instanceof Backbone.Model || obj instanceof Backbone.Collection) { + return; + } else if(_.indexOf(visited, obj) > -1) { // Don't choke on circular references return; } else { visited.push(obj);