jashkenas / backbone

Give your JS App some Backbone with Models, Views, Collections, and Events
http://backbonejs.org
MIT License
28.09k stars 5.38k forks source link

Providing a bare bones convention for nested views #2490

Closed tgriesser closed 11 years ago

tgriesser commented 11 years ago

The biggest source of questions/complaints/issues I've encountered with Backbone deals with handling the issue of nested views. Any sufficiently complex application (see: real world applications) has a need for managing views within views, and cleaning up after them properly. The (well received) addition of listenTo helps with this to a degree, but I think Backbone could do a bit more.

This is something I add to pretty much whatever I'm working with, and it does the job without much overhead or complexity:

var View = Backbone.View;
Backbone.View = Backbone.View.extend({

  constructor: function() {
    this.subviews = [];
    View.apply(this, arguments);
  },

  addSubview: function(view) {
    if (!(view instanceof Backbone.View)) {
      throw new Error("Subviews must be a Backbone.View");  
    }
    this.subviews.push(view);
    return view;
  },

  removeSubviews: function() {
    var children = this.subviews;
    for (var i = 0, l = children.length; i<l; i++) {
      children[i].remove();
    }
    this.subviews = [];
    return this;
  },

  remove: function() {
    this.removeSubviews();
    View.prototype.remove.apply(this, arguments);
  }
});

I completely understand and agree with Backbone's position to keep things simple and not implement applications specific components; at the same time I think providing a very, very basic starting point for this problem would be a great help to many.

This starting point could then be extended upon with more opinionated libraries or frameworks like layoutmanager, thorax, marionette, etc.

/cc @tbranyen

tiye commented 11 years ago

+1 Nesting views is so much complex thing in Backbone.

braddunbar commented 11 years ago

Mornin' Tim! While I can certainly understand the impetus for this change (I've written similar code many times), I don't think subview management is appropriate for inclusion.

Backbone Views are almost more convention than they are actual code.

Powerful primitives and good conventions to build upon. The code above is a rather small amount of boilerplate and doesn't address any hard issues like disposal and data binding that people mostly don't agree about. However, it implies that Backbone should handle those when it shouldn't. Further, many views don't contain other views and allocating an array for them is silly.

Leaving this unimplemented allows for creativity in solving hard problems and working with Backbone instead of around it.

jashkenas commented 11 years ago

The other nasty thing about this pattern is that it stores references ... meaning that you now have to go back and manually remove your child views.

In a more agnostic setup, if the HTML for your subviews is removed at the same time that the models stop being referenced, it all just gets garbage collected for you. The more unnecessary references you have in JS, the less garbage collection gets to do its job. We should try to have as few as possible mandated by core Backbone.

tgriesser commented 11 years ago

... allocating an array for them is silly.

Good point, this could be simplified to (this.subviews || this.subviews = []).push(view)

The other nasty thing about this pattern is that it stores references ... meaning that you now have to go back and manually remove your child views.

If you call remove on the top level view, it then calls remove on child views automatically, and you're good... it doesn't seem too nasty.

... working with Backbone instead of around it. ... as few as possible mandated by core Backbone.

Makes sense, and I agree... I really only brought it up because I've heard so many questions about it (probably just behind nested models/collections).

It might be worth mentioning something small in the docs about ways to deal with views within views, for the cases when the html for subviews aren't removed at the same time as other objects the view maintains references to. Any opinions on that?

tbranyen commented 11 years ago

Allocating an array isn't silly, bringing it up as a valid reason for not doing something is though.

@braddunbar https://github.com/pathable/quilt/blob/master/quilt.js#L28 ?

tbranyen commented 11 years ago

"Maintaining Views is, in my opinion, the most difficult part of a Backbone application because you must make sure that you properly dispose of your views, detach events, dereference objects, etc or you absolutely will run into memory leaks."

http://deserialized.com/javascript/our-experience-with-backbone-js-and-why-were-considering-angularjs-as-a-replacement/

This post is from today and there are many more like it. In my opinion it is not a contrived problem. Other than plugins and blog posts, what is the best way to make this library effective for creating Views to newcomers, in the same way that Models and Collections attempt to provide? I would love to work on Layout Manager to the point where it could be merged, but I fear it would be an effort wasted.

If you don't mind me asking, what would be the main objections, other than "we don't want it", that you'd have towards integrating well tested and properly handled View management?

tgriesser commented 11 years ago

I think one of the main issues that @tbranyen touched upon is that Backbone views are typically promoted as jQuery, with better structure... keep your logic/state out of the DOM, all of that good stuff.

The problem is that when you're coming from $ land, you don't have to think about references and gc (almost) at all, the black box handles cleaning up references for you in a big way. With Backbone, there isn't much done for that outside of models/collections.

If you're dereferencing your models and views simultaneously (as @jashkenas often mentions), then you'll end up with more "loading" screens and server hits than your application may have needed if more data was kept around in memory... There are many valid cases where a view would be observing events on an object that would outlive it - or if subviews are added in a view's "render" block, referencing the parent's model, and the parent view is re-rendered, you now have views that hang around until the parent model is gone, unless you explicitly kept track of and removed the views at the beginning of the render block...

We should try to have as few as possible mandated by core Backbone.

Also, looking at this comment again I have to say that I agree fully, but I also think there's a big difference between "mandating" something and providing an outlet for handling something that's rather common and a major pain point and source of confusion, in a simple and entirely optional fashion.

If the 70 cloc Backbone "view" section is as complicated as it's going to get (vs. the 473 for models and collections, not counting underscore)... that's fine, but I can certainly understand why nested views aren't immediately obvious and have sprung so many blog posts, half baked plugins, and frustration.

braddunbar commented 11 years ago

@tbranyen You're right, that wasn't a great reason and I shouldn't have mentioned it. I think my main point still stands though. I've been known to do all sorts of silly things, not that I recommend them all. :)

If you're dereferencing your models and views simultaneously (as @jashkenas often mentions), then you'll end up with more "loading" screens and server hits than your application may have needed if more data was kept around in memory

I've done this both ways and I've found it to be a trade off. Keeping things around in memory can improve perceived load time, but is often accompanied by a high cost in complexity and cache management (leaving view disposal/management out altogether is a huge win). Both are valid strategies, and the choice depends on your constraints.

jashkenas commented 11 years ago

I still don't think that we should be merging a change like this. To repeat myself -- if we maintain references to subviews in Backbone core, then suddenly lots of folks that previously didn't have any problems working with GC properly now have to start manually removing every subview ... a feature regression -- and a possible initial step down the (possibly) wrong path for beginners.

That said, there are of course many cases where you do want this style of view hierarchy. Let's solve this with documentation. Anyone want to cook up a nice "Nested Views" section for the homepage? Talking briefly about 1) how to make your views flow nicely with GC, and then 2) how to maintain an explicit hierarchy with automatic granular unbinding on remove, demonstrating the code above?

tgriesser commented 11 years ago

That'd be great - I can take a shot at putting together some of those docs.

jashkenas commented 11 years ago

In addition, the docs needn't include all of the code above. Maybe make it a tgriesser/backbone-nested official plugin, and link over to the reference implementation?

dgbeck commented 11 years ago

Is the above code intended to manage a fixed number of subviews or to manage a variable number of subviews? In my opinion those are fundamentally two different beasts and they warrant totally separate solutions from the start.

If you split up a section of UI into separate subviews for organizational / code reuse purposes, then you have a fixed number of subviews. It is useful to reference those subviews with names. A hash keyed by subview name is most appropriate data structure for storing the subviews in this case.

In contrast, if you are rendering a collection, then you have an arbitrary number of subviews and an array is more appropriate for storing these subviews.

The code above is not a sufficient solution for either case, IMO. The first case is simpler, but even with that case, there is one question that immediately arise and need to be addressed. Are the subviews re-initialized when the parent is re-rendered, or are they just re-rendered? For most cases you just want them to be re-rendered. But it is non-trivial to replace the parent HTML without loosing the events that are attached to the subview DOM elements.

The "minimal" solution for rendering a fixed number of subviews that we have been able to distill is the Backbone.Subviews mixin. Any solution that does not address the re-rending issue, IMO, is incomplete.

The case of a variable number of subviews is another matter. I will not go into that more complicated case but to add the new Backbone.CollectionView plugin as one of the available solutions.

tgriesser commented 11 years ago

Are the subviews re-initialized when the parent is re-rendered, or are they just re-rendered?

I actually find that in the majority of cases if the parent is re-rendered, it is easiest to both re-initialize and re-render the view... in which case the above should work just fine (as long as you call removeSubviews in the topmost render)

When splitting up the UI, I many times won't use a hash or named subviews, as the subviews will re-render themselves based on model changes, etc so I don't need to reference them from the parent at all... But I guess this just confirms what Jeremy said, that there is no one size fits all scenario for view management, and is something that shouldn't be addressed of in Backbone.

wyuenho commented 11 years ago

if we maintain references to subviews in Backbone core, then suddenly lots of folks that previously didn't have any problems working with GC properly now have to start manually removing every subview ... a feature regression -- and a possible initial step down the (possibly) wrong path for beginners.

@jashkenas I don't quite understand this sentiment. Suppose you only have to one view, the proper way to remove it other than closing the tab is to call View#remove. You have to document this somewhere somehow with a big bold letter that says DONT JUST USE $.fn.remove() and $.fn.empty() OR GET MEM LEAKS anyway. @tgriesser 's suggested change is a good start and actually will simplify memory management by automatically propagating the remove call.

I don't know how many beginner's View code you have seen, but from what I've seen, beginners will absolutely, definitively, not get it (it being the so-called structure). For people whose brains had been warped by jQuery for half a decade, the jQuery chain IS the structure. They don't like it, so they look for guidance in Backbone but Backbone offers next to no help and is refusing to do so. The single most likely place to induce partially digested stomach content ejection jQuery code is the lack of a blessed way to nest views. They will put code that belongs to the subviews in the parent view, implement half-ass solutions to propagate Backbone events from deeply nested subviews to the ancestors and many times just a big blob of jQuery pasta within render because they don't know how to deal with subviews. After about 2 years with Backbone, a lot of people are starting to converge towards the solutions suggested herein. I sincerely hope you can reconsider trying to hash out a View class that solves most of the problems people have with the current implementation.

It is useful to reference those subviews with names. A hash keyed by subview name is most appropriate data structure for storing the subviews in this case.

@dgbeck Actually a hash is a sufficient and appropriate solution for all situation since if you are rendering a collection of views, the keys can simple be the index. You just have to make sure the subviews come out in the right order. @caseywebdev et al. 's book actually suggests using a hash.

Are the subviews re-initialized when the parent is re-rendered, or are they just re-rendered? For most cases you just want them to be re-rendered. But it is non-trivial to replace the parent HTML without loosing the events that are attached to the subview DOM elements.

Most cases the parent will just call $.fn.empty() and re-render the subviews, and in each subview's render method, the DOM events will be re-delegated. Yes this is a lot of work (as in CPU cycles) done and makes Backbone's View even more tightly bound to jQuery. I'd love to see a clean implementation that actually downgrades jQuery importance to be side-by-side with just the DOM API for many reasons, but that's a whole other blog posts...

steverandy commented 11 years ago

Actually backbone could just use "views" hash to track child views and provide few extra methods to make view management easier. This is what I did. https://github.com/steverandy/backbone.managedview/blob/master/backbone.managedview.coffee

RStankov commented 11 years ago

I am actually interested in what approaches people are using in real software, not theory realms. Since a lot of people are solving this problems, and there are probably similar patterns. I have used two patterns, which I liked:

Backbone.Support approach, I like like it because the view rendering is included in the inclusion of subview.

The other way I have used subviews is via my own Backbone.Handlebars /which I don't try to promote or anything/. But the idea there is to hide the subviews into the templating engine. There is a method to access the subview via a obscure renderedSubViews, but I can't recall ever use it.

@jashkenas One possible solution about removing a subview from imaginary this.subviews is to extend Backbone.View with Backbone.Events and have rendered, removed, subview:added, subview.removed and so events. I understand that subview rendering is very very dependent on the tempting engine. But for me the one biggest Backbone weaknesses (and strengths) is that it doesn't have a clear template engine pipeline.

wyuenho commented 11 years ago

@RStankov Subview rendering does not depend on template engines at all. You can perfectly nest views, where each view only represents 1 HTML element constructed by jQuery in View's constructor. Backbone.Support does not address @dgbeck 's issues because it uses an array. Sometimes you need to be able to name your subviews. I also don't see how giving extra events is going to help as there's not an obvious way to construct a view hierarchy in Backbone. Backbone events also don't bubble like the DOM's. Events are only useful when all your subviews are sharing the same model/collection.

If you want to look at real world code, here's Backgrid. The whole thing is entirely composed of nested views using a slightly less elegant variation of @tgriesser 's solution. This is sufficient only because I happen to have no need to name my subviews, but many other situations will certainly call for a hash. It really should be the default way to go as Underscore can trivially make a hash to behave like an array.

RStankov commented 11 years ago

@wyuenho Nice grid :)

My point was mainly that we should look for patterns used in projects, since I noticed everybody handling this is a different way. And I'm more and more moving to have subviews in template helper and see the subviews in the markup not in the view code. But I understand this my pattern and is not the right solution for generic stuff.

The need for view event like removed is that if you have a subview. And its model is removed this triggers removing of the subview, it will be removed from DOM, but will still be collection of its parent view. The other way is to have reference for parent view into the subview and upon removal to remove the child from parent.

Most of the places I take heavy use of subviews, the hard part is creating them and putting them into the correct DOM places. Then they self managed, and the way they communicated with their parent was with custom DOM events. And I can't remember a case where the parent needed to call a method of a subview.

acstll commented 11 years ago

@RStankov I'd like to follow your comment and share the way I handle nested views. Backbone.View already inherits Events.

I add or mix in these to methods to all my views I know will be nested/parents:

dispose: function () {
  this.trigger('dispose', this.cid);
  this.remove();
},

register: function () {
  if (this.options.parent) {
    this.listenTo(this.options.parent, 'dispose', this.dispose);
  } else {
    console.log('Tried to register child view with no parent:', this);
  }
}

Then whenever I instantiate a 'child' view inside another view I pass the parent as an argument { parent: this }. And on the child view's initialize function I call the register method.

No hash or anything. Every child view holds a reference to its parent and listens for a 'dispose' event. When a parent view gets disposed, all child views do so as well.

dgbeck commented 11 years ago

Catching up on this thread -

Most cases the parent will just call $.fn.empty() and re-render the subviews, and in each subview's render method, the DOM events will be re-delegated.

@wyuenho, DOM events are delegated when views are initialized, not when they are rendered. You will loose your DOM events on subviews if you simply re-render subviews when the parent is re-rendered. A work around is to detach them and then reattach them to the DOM. (If you take the other approach discussed, re-initializing views after the parent is rendered, you loose all state data in the subviews, for which I know of no easy work around.)

I agree with you that keeping references to subviews within the parent should not be a deal breaker as long as subviews are removed when .remove() is called on the parent. @jashkenas this seems pretty low cost as it is already a requirement that people call .remove() in order to that their views be properly garbaged collected.

I also don't see how giving extra events is going to help as there's not an obvious way to construct a view hierarchy in Backbone. Backbone events also don't bubble like the DOM's.

Try Backbone.Courier. You'll like it, I promise =).

@RStankov your good point about needing an event to remove a subviews from the parent only applies to case (2) below, I think. In case (1) subviews are almost never removed, and when they are one could just re-render the parent and the parent could then drop any old references to removed subviews.

@acstll if the approach you described works for you then go for it but keeping explicit references to a parent view in a child view is generally not a good idea.

To recap:

Thanks @tgriesser for starting this thread.

tgriesser commented 11 years ago

Well typically 1 and 2 go hand in hand, you're splitting up a view into smaller components because you have different models/collections for organization/code re-use.

Just to illustrate, this is the classic example of an incorrect implementation (going off the nested models/collections example in Backbone docs) of where I think Backbone could provide a little bit of guidance, whatever that may be:

class MailboxView extends Backbone.View
   initialize: ->
      @listenTo(@model, 'change', @render)
   render: ->
      @$el.html JST['/some/template'](@model.toJSON())
      @$('.some-class').append(new ListView(collection: @model.messages).render().el)
      this

class ListView extends Backbone.View
   initialize: ->
      @listenTo(@collection, 'add', @addItem)
      @listenTo(@collection, 'reset', @render)
   addItem: (m) ->
      @$el.prepend(new MessageView(model: m).render().el)
   render: ->
      @collection.each (m) =>
        @$el.append(new MessageView(model: m).render().el)
      this

class MessageView extends Backbone.View
   initialize: ->
      @listenTo(@model, 'change', @render)
      @listenTo(@model, 'destroy', @remove)
   render: ->
      @$el.html JST['/some/template'](model: @model)
      this

So here, imagine the mailbox model changes, maybe you changed the "last_fetched" attribute on it, but the collection remains intact as a property on that model - thus the collection/ associated models outlive their views...

It won't make a huge difference here because there's not much going on in the child views' render, but it ends up being an issue in a lot of cases, and leads to frustration if you don't understand why.

This would then be easily solved, by adding @removeSubviews() in the top level render function, and by wrapping each child view's constructor with @addSubview.

I didn't write up the docs for it yet because I wanted to hear a bit more discussion, but here is the plugin I'm thinking should handle a good bit of this confusion... it's really only two very simple functions, though... I still think it is something that could be considered for addition to Backbone directly.

Also, looking back through the thread at @braddunbar's comment...

Leaving this unimplemented allows for creativity in solving hard problems and working with Backbone instead of around it.

In my opinion, this is such a boilerplate situation, I don't see why it needs to a) be a hard problem and b) be solved creatively by each plugin differently, each time. If it needs to be done creatively, nothing is forcing this implementation, but I think Backbone could provide something for the 90%+ case.

P.S. - @dgbeck nice stackoverflow link (asked by me, about a year and a half ago)... It wasn't immediately obvious how view relationships should be handled when getting started (the answer being, there isn't really an answer), which is probably why I brought it up.

acstll commented 11 years ago

@dgbeck , @tgriesser thanks. I'm glad about posting my aproach, because it demonstrates there's some of us doing things wrong after so long. I agree:

If it needs to be done creatively, nothing is forcing this implementation, but I think Backbone could provide something for the 90%+ case.

wyuenho commented 11 years ago

DOM events are delegated when views are initialized, not when they are rendered. You will loose your DOM events on subviews if you simply re-render subviews when the parent is re-rendered. A work around is to detach them and then reattach them to the DOM. (If you take the other approach discussed, re-initializing views after the parent is rendered, you loose all state data in the subviews, for which I know of no easy work around

@dgbeck I know :) What I mean is when I re-render a parent view I usually do something like this:

render: function () {
  this.$el.empty();
  this.removeSubviews();
  // ...
  // render parent view..
  // ...
  this.renderSubviews();
  this.delegateEvents();
}

This, of course, only works if all the subviews inherit from a View superclass that implements this.

The way communication goes in my view hierarchy is that I always make sure they all share the same collection/model and I simply trigger custom namespaced events from the shared collection/model like so:

model.trigger("namespace:event", param1, param2, ...);

Interested parties can just listen to those events and act accordingly, so in a sense the collection/model is a reversed-DOM. I've never found the need to set up an event chain among my views or abuse the actual DOM to propagate Backbone events.

I agree with @tgriesser that your 2 cases typically go hand in hand.

I happen to be working on two projects that use nested views quite heavily and have two custom View classes. One is similar to @tgriesser 's backbone-nested, but uses both a hash and an array to keep track of subviews. Another one offers a jQuery-free mode but 100% compatible with the existing View, but I digress... Might open source some plugins and file some PRs this weekend...

wyuenho commented 11 years ago

BTW, @jashkenas is there policy in this project that sets up a path to merge good plugins (for some definition of good) into the core or is the policy once a plugin, always a plugin?

jashkenas commented 11 years ago

I'd like to think that there are no policies -- at least formulated as such...

If a plugin is a good idea for core, then we should merge it. 'Nuff said.

dgbeck commented 11 years ago

@tgriesser thanks for the repo. Definitely helps make the discussion more concrete.

There are two problems with the code as it is that would prevent us from using it, even for the simpler case of splitting up a view into smaller subviews.

First, we find it useful to name subviews. In the above Mailbox example, what if the MailboxView wants to listen to an event on the ListView? Or what if it wants to tell the ListBox view to collapse all messages when a collapse button the MailboxView is clicked? In this case it is useful to be able to reference it by name:

this.listenTo( this.subviews.listView, "messageSelected", this._onMessageSelected ); 

or

this.subviews.listView.collapseAllMessages(); 

This problem can be solved easily by storing the subviews as a hash. Here is a fork of the code with one incarnation of that change. In this fork people who want to supply the keys for their subviews can do so, and if no keys are supplied the behavior will fall back to using numbered indexes as hash keys.

Second, our views have state information and we can't loose that information when the parent view is re-rendered. (Nor does it seem very efficient to re-initialize subviews each time a parent view is re-rendered.) Are we the only ones who store state information on our views? For instance, again using the above example, the listView might contain state information about which message(s) are selected. Storing that information as a selected attribute of on messages themselves does not seem appropriate to me because it is not persisted to the server nor shared among different users. In my mind this kind of temporary "view state" information belongs in the view layer, not in models.

This second problem is harder to solve simply but maybe it could be left for plugins. I could not find a way distill a solution further than Backbone.Subviews. However, if hash version of backbone-nested was integrated into core we could re-write the Backbone.Subviews plugin on top of it.

Thanks again for the repo and for your efforts in helping people through this common issue.

tgriesser commented 11 years ago

So to address those two points...

1) I agree that named views are good to have... but in many cases you don't need them... Because addSubview returns the view that was added, you can easily attach the subview to the parent view like so:

this.listView = this.addSubview(new ListView(collection: this.model.messages))
this.$('.some-class').append(this.listView.render().el)

In my opinion this feels a bit cleaner (this.listView vs this.subviews.listView)... and still maintains the same necessary cleanup needed with removeSubviews.

2) In this case, since you're dependent on each of the views maintaining their own state, you probably just wouldn't want to let parent view(s) re-render at all... any only really use them for the structure they provide, but keep all of the child views as siblings and let them re-render independently. Then you don't have to worry about any of that.

dgbeck commented 11 years ago

1) I also prefer this.listView as opposed to this.subviews.listView but having references to subviews in two places does not feel quite right to me. It is reminiscent of the redundancy in having options attached to view itself and on view.options, but arguably worse because these are all object references and need to be cleaned up when no longer used. If you double up these references then view#removeSubviews no longer guarantees the subviews will be garbage collected. In many cases this may not be a big deal but it seems a little off.

Moreover there are other cases in which people may want to customize the keys used to store subviews. For example, in the second case of rendering a collection of models, you might want subviews to be indexed by the cid of the model object. That way you can easily find the view that corresponds to a particular model.

Why limit people's options? Is there a disadvantage of using a hash as opposed to an array? The _.size call is not ideal. Maybe there some better way to give each subview a unique index...

For (2), I think it makes more sense to solve this problem with the Backbone.Subviews approach, for our use case, at least. But the fact that we could build that approach on top of a hash-based nested-view plugin, and eliminate some code, seems like a testament to nested-view's versatility.

tgriesser commented 11 years ago

The removeSubviews is mostly about ensuring the stopListening is cascaded to prevent latent effects by leftover event listeners, so keeping a reference to the subview going down the chain isn't really a big issue, eventually it all gets gc'ed once its parent view is dereferenced or the reference is manually overwritten/deleted. If you need to reference it by name, you're probably going to be keeping it around for a bit anyway... probably at least until the view goes away, in which case you're fine.

Finding one by a cid wouldn't be too difficult in an array either:

subviewByCid: function(cid) {
  return _.find(this.subviews, function(view) { return view.cid === cid; });
}

My other thought is that if you're needing to reference it by name, you should be thinking about why you need to do that in the first place, the views should (in theory) be maintaining their state independently and shouldn't need to be referenced by parent views very often.

That, and arrays are faster.

wyuenho commented 11 years ago

Yep allowing lookups by cids is sufficient. There's no need to provide an explicit name.

I've just push NestableView. Please take a look. Design rationale is in the README and the source code comments.

https://github.com/wyuenho/backbone-views

wyuenho commented 11 years ago

what if the MailboxView wants to listen to an event on the ListView? Or what if it wants to tell the ListBox view to collapse all messages when a collapse button the MailboxView is clicked?

@dgbeck It is entirely possible for all the Views in this example to listen to the mailboxView.model.messages for events and only communicate by sending and listening to events on the shared collection/model. I find that when views start binding events on each other directly is exactly when you have inflexible tightly coupled spagetti code, this is most evident when you need to reorder you view hierarchy, all of a sudden you'll have to dismantle your entire event chain. Data model rarely change dramatically, you may have an extra field here and there, or maybe some extra events, you can just add a few lines in your render and/or sprinkle in some listenTos in your views to listen to new events. By and large your views are a lot more likely to change than your data model, you can insert, remove and reorder your view hierarchy, but the data that you need for any given problem is always going to be the same.

wyuenho commented 11 years ago

Second, our views have state information and we can't loose that information when the parent view is re-rendered. (Nor does it seem very efficient to re-initialize subviews each time a parent view is re-rendered.) Are we the only ones who store state information on our views?

I store state info in views all the time, but I keep my render methods idempotent so any rerendering, given the same view state will result in the exact same look and behavior. Reintializing subviews is not necessary if you can keep your render method idempotent. You can also use a template instead if deeply nested views are not desirable. Nothing in @tgriesser 's implementation nor mine prevents that from happening.

tgriesser commented 11 years ago

@jashkenas, so from a bit of discussion here, it sounds like this is definitely a real world issue that faces a lot of folks when building rich ui's, there are plenty of good cases where you'll want a view to listen to an object that is a bit longer lived - hell, Backbone is now advertised as a global event bus, so any views that may subscribe there need to be explicitly removed... The issue for me isn't as much about gc as preventing latent effects from zombie event listeners (which in-turn helps with gc).

As you pointed out:

... if we maintain references to subviews in Backbone core, then suddenly lots of folks that previously didn't have any problems working with GC properly now have to start manually removing every subview ... a feature regression -- and a possible initial step down the (possibly) wrong path for beginners.

I think the truth is quite the opposite - beginners don't realize that you do need to keep track of views when they listen to objects outside of their life cycle (see: #2518), and as long as the removing every subview goes down the chain from the top level view removal, you don't have to do any manual removal.

Also, this would be an opt-in feature, so if you had a view with models/collections that live and die together, there's nothing forcing the use of an addSubview method, and if you're not using it, there's minimal overhead in the if (this.subviews) this.removeSubviews(); line in view.remove compared to the dom manipulation it's about to go do...

So, in my humble opinion, this wouldn't be a complex addition and would be quite helpful both in explaining why you might need to do this, as well as providing the minimal way to go about it. I think it would be as well received as the addition of listenTo / stopListening (which has been great).

dgbeck commented 11 years ago

Finding [a model] by a cid wouldn't be too difficult in an array either

Yes, but it's not as easy as in a hash, and trading a few O(1) lookups for O(N) lookups will quickly erase any performance edge that arrays have over hashes. (Interesting stack overflow.)

My other thought is that if you're needing to reference it by name, you should be thinking about why you need to do that in the first place, the views should (in theory) be maintaining their state independently and shouldn't need to be referenced by parent views very often.

If only the damn world would follow our theories =).

Also, I think was mistaken at the beginning of this thread when saying an array is the most appropriate view data structure for case (2) of rendering a collection of models. Even in Backbone.CollectionView, which uses Backbone.Babysitter internally to manage views, views are stored in a hash. Order is determined by the model collection, which is sufficient.

I can speak only from our experience and as we've seen there are lots of different approaches to this issue. (@wyuenho thanks for sharing yours).

If this plugin was integrated into core with an array-based implementation, we would end up working around it, not on top of it. If it was implemented as a hash, we could use it for case (1) of splitting one view into a fixed number of subviews, and we might also be able to use it for case (2) of rendering a collection.

It is clean and small, and it seems like a solid foundation for the majority of cases.

wyuenho commented 11 years ago

Summary so far:

  1. addSubview, removeSubview is necessary.
  2. Removing subviews automatically inside remove is a good idea.

TBD

  1. Should addSubview, and removeSubview support adding and removing a list of views?
  2. Should render provide some minimal boilerplate to render the subviews and the parent view like NestableView?
  3. Should we use a hash or an array or a combination of hashes and arrays for internal storage?
  4. Should we provide any help to facilitate view communication via events?
  5. Should subviews all get a reference to their parent views?
  6. Related to 3), should we provide a way to lookup views by index/key/cid?
My preferences:
  1. Yes for addSubview, leaning to yes for removeSubview unless the choice of internal storage ADT makes this too slow.
  2. Yes. People who don't care about subviews can continue to override render and it will continue to work, people who do can choose to override something else like renderView in my implementation.
  3. Hash for fast lookups and lookups by key, possibly just the view cid, but need a way to retain insertion order.
  4. No, but encourage the sharing of collection/model within the same view hierarchy and the use of custom namespaced events triggered from collection/model. This seems to be the easiest to do without using any extra plugins or trying to coerce the DOM to propagate Backbone events.
  5. I haven't found this necessary in my experience and causes more trouble then it's worth, but if 4) is left for the users, maybe at the minimum we should document the various ways to set up cross-view communication.
  6. No preference at this point, really depends on the choice of data structure in 3).

Thoughts?

lfac-pt commented 11 years ago

Hi,

This is quite a long thread (and I haven't been able to read all of it, sorry), nevertheless I wanted to give my two cents in this mater because in the project I work we have made our own implementation to solve this problem (we call it FatherView).

The implementation shares a lot of aspects with some of the things that were said before. Some key points:

I'm not saying this is the best or most correct way. My objective was to explain our use case and maybe have some feedback on how our approach can be improved.

tgriesser commented 11 years ago

addSubview, removeSubview is necessary.

I don't think they've been deemed necessary yet, as it's definitely possible to get along without them - and everyone has a bit of a different take on it. It's just something I think could be helpful in building more robust model/collection structures where you're bootstrapping nested models/collections not just grabbing/dumping individual models/views each times things change.

@lfac-pt - thanks for sharing, that sounds a bit closer to something more opinionated like layout manager with the swapping of views, etc... but it sounds like it shares some similar features as the other approaches mentioned here, namely that the parent stores child views, and has the ability to remove them in one way or another.

wyuenho commented 11 years ago

addSubview and removeSubview are not necessary for casual single view usage, but necessary to move this discussion forward :)

Seriously, it does look more and more like we are trying to reimplement backbone.layoutmanager tho...

lfac-pt commented 11 years ago

Well, I can say that the reason that we opted by our FatherView instead of backbone.layoutmanager (or something similar) is that at 24 LOCs our little abstraction is very easy to understand and yet allows us to keep our code clean and simple.

That is one of the reasons why we use Backbone, it's abstractions are simple and easy to understand and yet very powerful. IMHO a plugin for nested views should also follow this philosophy.

lfac-pt commented 11 years ago

OK, actually with jsdoc and requirejs boilerplate its almost 50LOC :P

dgbeck commented 11 years ago

I think @tgriesser nailed this from the beginning. I don't think you can do any better in terms of utility per character. I do think a hash is the better choice on the grounds of ease of use, performance, and versatility. @wyuenho your idea of using view.cid as the hash keys is a perfect default, guaranteeing unique keys. People could also have the option to supply their own keys, so that they can lookup subviews by name, or model.cid, or whatever works best for their case. Here is Tim's original code with the hash tweak that gets my vote for best candidate for core integration:

https://github.com/dgbeck/backbone-nested/blob/master/backbone-nested.js

This is still a partial solution, at least for our use cases, but it is a solid building block. The more sophisticated solutions presented in this thread could be built on top of it. Under-shooting rather than over-shooting seems to fit in well with the rest of Backbone. This is a relatively small amount of code that provides a lot of helpful structure.

andreypopp commented 11 years ago

I suggest use view instead of subview in terminology when talking about nested views. I think sub- prefix introduces unneeded verbosity.

acstll commented 11 years ago

@andreypopp the action names "add" and "remove" could also be argued. Naming seems to be very important in Backbone. listenTo and stopListening could be good examples of that (I remember reading somewhere around here @jashkenas explaining the logic in it). For "adding" you could also use set push or register, I don't know.

wyuenho commented 11 years ago

@dgbeck The fact that a JS hash doesn't retain insertion order really concerns me because the ordering of subview does matter for lots of application. Lots of applications can't guarantee the irrelevance of subview ordering, so using a hash as the only ADT storage is insufficient, as least for Backgrid.js. I would really really prefer to store the subviews in an array to preserve the insertion order AND a hash for fast lookups by cid if necessary.

@acstll I agree that add and remove are probably more succinct names. What else can you be adding when you call View#add ? :)

philfreo commented 11 years ago

@acstll I agree that add and remove are probably more succinct names. What else can you be adding when you call View#add ? :)

It could easily be confused with adding other objects (models, data points to a chart, non-view dom elements, etc). Don't forget views can be used in all sorts of creative ways right now.

wyuenho commented 11 years ago

@philfreo You could argue the same for Collection no? Those methods never existed on View, why would they be confused? You can still do anything you want with this change.

philfreo commented 11 years ago

A collection is specifically meant to hold models.

I'm not against subview management per se, just pointing out that View#add isn't completely obvious and without possibility for confusion, due to the fact that the primary purpose of a View isn't to hold other views.

wyuenho commented 11 years ago

I see. I'm fine with either way actually.

acstll commented 11 years ago

…just pointing out that View#add isn't completely obvious and without possibility for confusion, due to the fact that the primary purpose of a View isn't to hold other views.

@wyuenho @philfreo Exactly. But also that "adding" could lead beginners to think about rendering or even the DOM. When the case here is just keeping a reference in a hash. And this is what I wanted to point out, nothing else. :)

andreypopp commented 11 years ago

@wyuenho @acstll @philfreo I asked only about dropping sub suffix — so I'm ok with addView, removeView instead of addSubview, removeSubview.