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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fcamblor
Copy link
Contributor

@fcamblor fcamblor commented May 6, 2015

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

afeld commented May 7, 2015

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?

…sted Backbone.Model when we use Backbone.NestedModel
…it will generate lots of

useless observers, particularly on Backbone.Model._events which may contain huge event binding data
@fcamblor
Copy link
Contributor Author

fcamblor commented May 7, 2015

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

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

@rodneyrd
Copy link

rodneyrd commented Apr 19, 2016

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!

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.

3 participants