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

Tracking collection state. #48

Closed
wraithgar opened this issue Jan 28, 2015 · 13 comments
Closed

Tracking collection state. #48

wraithgar opened this issue Jan 28, 2015 · 13 comments

Comments

@wraithgar
Copy link
Contributor

A collection emits events that are bubbled up from the models it contains. This is the current behavior and expectation from even pre-ampersand (i.e. backbone).

It would be nice if there were a way to event on collection state events, specifically the length. Unfortunately you can't just start emitting change:length events because everything listening to events coming out of the collection would have to start paying attention to the type of object emitting the change event, because it could now either be the collection itself or a model it contains.

There are two ways to potentially solve this.

  • emit collection state events from a different namespace, i.e. instead of change collectionChange
  • attach a state object to the collection that will hold and emit these collection state attributes somewhere like at collection.state

The latter feels more properly separated but this is something a lot of people have asked for (i.e. being able to derive an empty attribute) and I don't see any discussion happening lately so I'd like to get this out there.

@wraithgar
Copy link
Contributor Author

One other option would be a mixin, however it would require the end-user to initialize the mixin code as part of the collection's setup.

@adamyonk
Copy link

I like the sound of a state object on the collection. I think this could be really useful. 👍

@cdaringe
Copy link
Member

dont add remove imply a length change? maybe the events just need to pass additional meta?

@wraithgar
Copy link
Contributor Author

@cdaringe reset changes length too

@cdaringe
Copy link
Member

on add remove events, could you just check the passed model's modelChanaged.collection.length (if .collection is set), and on reset assume length 0? actually, this wouldn't work when a model is shared across collections.

new idea. perhaps we pass more than just the model to event handler--pass a {collection: collection, model: model} hash as the context. this would break a lot of code :/

@imlucas
Copy link

imlucas commented Apr 3, 2015

@wraithgar Before diving in, I'm guessing there's some history and discussion of why ampersand-collection doesn't just inherit from ampersand-state like everything else? (Guessing this is because of API conflicts like get)

@latentflip
Copy link
Contributor

I've discussed this a bunch with @HenrikJoreteg and others in the past, and even got as far as getting something working in a branch: https://github.com/AmpersandJS/ampersand-collection/compare/based-on-state

The last time I thought about this where I think got to was:

  • Ampersand-collection would inherit from state.
  • models would effectively be a property of the collection, changes to the models array (add remove reset etc) would trigger a change:models event on the collection.
  • length is then a simple derived property of models
  • you could then easily define other derived props on a collection (like totalling values on models in the collection easily)

The outstanding questions were:

  • as @imlucas pointed out: state has get/set methods and so does collection. These do different things, and both would potentially be required if we allowed a collection to have props/session attributes too (in that branch I just renamed it collSet to get something working.
  • If you just rethrow individual model events from the collection as is, e.g. change:name then you now have no way of knowing if that's a change on an item in the collection, or a change on the collection itself. I think my plan was to rethrow model events as, e.g. change:models.name. But that's obviously a fairly significant change to how things work currently.

@erd0s
Copy link

erd0s commented Apr 8, 2015

I'm trying to wrap my head around this whole situation and it seems to me that extending from state adds a lot of extra stuff to collection that we don't really need, also it seems like having a state object ask whether it has actually become a collection is a bad programming practice (I can't explain why, and I could very well be wrong!).

What about if we had another dataType in state which was 'collection' instead of having collections as its own separate thing. That way you could have deps refer to a property of type 'collection' to know when to recalculate and cache the derived property? Could it work like an array? It just bubbles up a change when something in the collection changes?

@imlucas
Copy link

imlucas commented Apr 9, 2015

@latentflip thanks! Still chewing on this a bit. Here's an example (using a gnarly "what if there was an internal _state that proxied?" method) which is what first got me into this topic.

@codepunkt
Copy link

@latentflip the direction your branch is heading seems promising. in my use-case, a collection has to have information about pagination, sorting and verbose filtering of items - so extending from ampersand-state would help me out a lot.

At the moment, i'm simply adding additional Models as properties of the collection.

@shaharmor
Copy link

+1 on extending &-state.
the change:models.name way sounds good too

@mmacaula
Copy link
Contributor

+1 on adding properties to collection. A lot of boilerplate is generated without it. However, I also agree with @erd0s, seems like by extending from state we'd be inviting a whole lot of weirdness. Collections could then have collections, or children as an example, and would seem to lose their purpose.

Perhaps we would want to lift only certain parts out of state, props, and derived props stand out. I realize how much code that actually means bringing over though.

@latentflip, I noticed that in your branch you have a deps on area for the collection, but I suspect it doesn't actually respond to individual changes on area for models. I only notice because this is the exact same mistake my team and I make all the time with derived properties. They may mean nothing but the code just silently ignores them.

Would 99% of desire-ments for this be solved with an evented property for length and a tutorial for people on defining their own properties on collections like @imlucas has done or just using composition as @gonsfx is doing?

My two cents...

@mmacaula
Copy link
Contributor

I just realized I could be wrong about my comment about the code not responding to change of model area events. I forgot derived events trigger off of 'change:property' and that's exactly what collections trigger. Sorry about that @latentflip!

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

No branches or pull requests

9 participants