afeld / backbone-nested

A plugin to make Backbone.js keep track of nested attributes - looking for maintainers! https://github.com/afeld/backbone-nested/issues/157
https://afeld.github.com/backbone-nested/
MIT License
444 stars 83 forks source link

Avoiding to checkchanges on sub Backbone.Models/Collections #139

Open fcamblor opened 9 years ago

fcamblor commented 9 years ago

While using Backbone.ModelBinder in correlation to backbone-nested, I encountered some weird chrome errors :

Failed to read the 'selectionDirection' property from 'HTMLInputElement': The input element's type ('time') does not support selection.

When I investigated further, I saw that backbone-nested was generating lots of checkChanges() calls due to paths looking like : myrootmodel.attributes.mysubcollection.models.0.attributes.mysubsubcollection.models.0._events.change:anAttribute.0.context.view.regionManager._regions.MyRegion.$el.prevObject.0.lastElementChild.lastElementChild.lastElementChild.contentWindow It was 1 of these calls which was calling an unauthorized call (from a chrome POV) on HTMLInputElement.

To fix this issue, I propose to avoid calling checkChanges() on nested objects of type Backbone.Model or Backbone.Collection : it seems reasonable to me since it will avoid to declare a huge amount of watchers on complex Backbone.Model hierarchy.

WDYT ?

afeld commented 9 years ago

Hmmmmm. The code looks good (thanks for all the documentation), but I'm wondering if there are other implications for why we wouldn't want to do this. If the object is a Model/Collection, should we be subscribing to its change events?

fcamblor commented 9 years ago

Yep, to my POV it would be the way to go because in any other way, it would be misleading for the developper.

If we bind changes on root models, direct changes on submodels won't be triggered :

rootModel.bind("change:mysubmodel.attributes.blah", myCallback);
rootModel.get("mysubmodel").set("blah", "bleh"); // Here, myCallback won't be triggered because we registered observer on rootModel, not on mysubmodel

On the contrary, if we bind changes to submodel, a change on the root model won't trigger it either :

rootModel.get("mysubmodel").bind("change:blah", myCallback);
rootModel.set("mysubmodel.attributes.blah", "bleh"); // Here again, myCallback won't be called because we didn't called mysubmodel.set()

=> I think this may lead to unwanted paths to be able to bind changes on submodels through rootModel (and vice versa). That's why I think it should be better to remove these observers and observe changes directly on models.

And I'm not even talking about performance here because you know, it's obvious that registering observers on things like myrootmodel.attributes.mysubcollection.models.0.attributes.mysubsubcollection.models.0._events.change:anAttribute.0.context.view.regionManager._regions.MyRegion.$el.prevObject.0.lastElementChild.lastElementChild.lastElementChild.contentWindow imply we're registering A LOT of observers behind the scenes, for unwanted (I think) things.

fcamblor commented 9 years ago

Is there anything I can do to clear things out if there are still things unclear ? :)

rodneyrd commented 8 years ago

As I am using backbone-nested with Backbone.Form & Backbone.Relational it can sometimes take up to half a min to render the form with a model containing nested Models/Collection because of those listeners. (cf screenshot, yes I have commented out the line manually). I tested it again with this pull request and it fix it. Could we merge it? Thanks a lot!