Skip to content
This repository has been archived by the owner on Apr 22, 2021. It is now read-only.

Firing dissociate events on collection reset #24

Open
pschyska opened this issue Nov 5, 2012 · 4 comments
Open

Firing dissociate events on collection reset #24

pschyska opened this issue Nov 5, 2012 · 4 comments

Comments

@pschyska
Copy link

pschyska commented Nov 5, 2012

Hey,

I have the following relationships

Foo.Models.Shift.has()
  .many "events"
    model: Foo.Models.Event
    collection: Foo.Collections.Events
    inverse: "shift"

Foo.Models.Event.has()
  .one "shift",
    model: Foo.Models.Shift
    inverse: "events"

In the Events resource, Shifts are referred to by shift_ids. When I reload the Events resource, and there are new events, they are properly associated to existing Shift models where the shift_id matches (I suppose the "associate" event gets fired).

When Events are missing though, there is no corresponding "dissociate" event fired, and the Shifts are not informed about Events needing dissociation.
What happens is that in my Events collection, and also in Foo.Models.Event._all, the Events that were missing from the last GET are not present.
However, the Event models are still present on the Shift models (via Shift.events()).

Note: The events are not nested within the Shifts in my JSON in this scenario. They are completely separate resources and we rely on Supermodel's association magic here.

Maybe one could

  • trigger the dissociate event on all models present on the collection, when it's being reset.
  • or detect if models are missing that have been there before when a fetch is called on a collection

I read some source code but I'm not sure where to attack this :-)

Thanks,

Paul

@pschyska
Copy link
Author

pschyska commented Nov 5, 2012

I put a hack in place to fire the disassociate Events on a reset manually for every .one() assoc with an inverse .many():

reset: ->
  @each (model) ->
    assocs = Foo.Models.Event.associations()
    assocs.shift.dissociate(model.shift(), model ) if model.shift()
    assocs.application.dissociate(model.application(), model ) if model.application()
    assocs.location.dissociate(model.location(), model ) if model.location()
    assocs.schedule.dissociate(model.schedule(), model ) if model.schedule()
    assocs.creator.dissociate(model.creator(), model ) if model.creator()
  super

It works as expected now.
I still think this should be done by Supermodel automatically, like the _a_ssociate events that are automatically fired.

@braddunbar
Copy link
Contributor

Hi @pschyska! Thanks for reporting this. You're right, this is definitely a bug. I originally punted on it because Backbone doesn't provide any information about the previous models in a collection. In fact, there is still an open issue describing it. The main issue is that Supermodel will now have to provide a collection implementation and require you to inherit from it.

I'll give this some thought and then take a stab at fixing it.

@pschyska
Copy link
Author

pschyska commented Nov 5, 2012

Exactly, I had troubles to implement it in Supermodel's source directly because backbone provides no "preReset" event.
I had some success monkey-patching Backbone methods with with ._wrap i.e. (pseudocode...):

Backbone.Collection.reset = _.wrap Backbone.Collection.reset, (original, models, options) ->
  @each (model) -> # when models given as param, iterate them instead of all models
    # disassociate model
  original(models, options)

I realize that you might want to go another way, justsayin :-)

A Collection implementation would also make it possible to refactor this model function boilerplate

model: (attrs, options) ->
  return Model.create(attrs, options)

... by providing a default implementation which does exactly this when saying

model: Model

@pschyska
Copy link
Author

pschyska commented Nov 5, 2012

I just read another referenced Issue with this and the comment given by @wookiehangover also applies to our case, I guess jashkenas/backbone#1521 (comment). When models are members of multiple collections (http://jsfiddle.net/wookiehangover/xCgNu/), and one of the collections is reset, it might not be the right thing to disassociate the models.

Maybe it should be done on fetch() only? Because fetch() result should be the canonical representation of this collection, and when there are some models missing in there it's safe do dissociate them.

Or, a clear warning about overlapping collections and dissociation in the documentation :-)

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

No branches or pull requests

2 participants