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

silent:false when setting child collections during initial set #90

Closed
wants to merge 1 commit into from

Conversation

lukekarrys
Copy link
Contributor

Came upon this when I was trying to fetch a collection of models, where each model had a child collection, but the initial add events were never being called.

I understand the reasoning that the constructor calls set with silent:true and I think that should be respected by passing it to child models and change events, but I was expecting add events to be fired on child collections. Am I wrong? Or is there a workaround for this that I'm missing?

@lukekarrys
Copy link
Contributor Author

@latentflip Any thoughts on this?

@lukekarrys
Copy link
Contributor Author

Spoke to @HenrikJoreteg about this last night, and here is the larger requirebin example I mentioned: http://requirebin.com/?gist=2ee1f57238558b111e93

this[attr].set(newVal, options);
continue;
// if it is a collection set silent to false during initial set
} else if (this._collections[attr]) {
this[attr].set(newVal, options.silent && options.initial ? _.extend(options, {silent: false}) : options);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only question is if we should just pass options through, here.

edit: errr... scratch that, i misread it.

@HenrikJoreteg
Copy link
Member

i'm curious what you were wanting to do with the add events in the app

@lukekarrys
Copy link
Contributor Author

I'm binding to add and remove from the child collection to trigger some session properties on the parent.

@HenrikJoreteg
Copy link
Member

I'm torn on this. Because arguably, you said you want it silently set.

I'm assuming you've got a method on the parent that calculates the total. Couldn't you just call that method in initialize on the parent?

@lukekarrys
Copy link
Contributor Author

Yeah, it can be done that way. I've been using my own style of derived properties for collections so I have deps: ['add', 'remove'] as a property on the collection, so that proper is off after initialization without manual triggering.

This is seeming to be a worse and worse idea anyways, so I'm not attached to merging this. It feels like too specific of a fix.

The only reason I'm torn is that the test I included seems like you'd expect it to work at first glance, and when it doesn't people will try and create workarounds (like doing a set after init, passing silent:false, etc).

@HenrikJoreteg
Copy link
Member

yeah, i sort of agree on the test, but i'm torn too. It feels like this is something that should be fixed by having a proper way to do stateful collections.

@mmacaula
Copy link
Contributor

Chiming in here, but wondering if this could be resolved by not doing a set on the collections with a pre-initialization step but just creating a new collection with the attrs being passed in? Then you'd get expected behavior on the children collections.
so:

  • remove _initCollections
  • update set to create a new collection with newVal, {parent : this} if the collection doesn't exist yet.

This might work for collections, but for children, perhaps something similar to account for _initChildren? I guess it comes down to whether the children and collections really need to be instantiated before being set or if they can be done all at once?

@HenrikJoreteg
Copy link
Member

thanks @mmacaula the only problem with that is that inside initialize we want the child collections to exist and not be replaced later.

@mmacaula
Copy link
Contributor

I see your point, don't want undefined collections in there ever, and maybe its too complex, but I think you could still have all your collections exist by initialize, and not be replaceable and still do a construction via the collection's constructor (vs a set).

That said, that might not solve @lukekarrys's problem, just mine. Looking over this again, I realize I'm bringing in my issue (recently closed) but which is a slightly different use case (new collections from a larger piece of json vs a model update of children via a fetch). I think they're sufficiently different that I might re-open that one.

@HenrikJoreteg
Copy link
Member

i think i'm going to close this, given the details outlined. It feels like the right solution is to have a better answer for state on collections. Feel free to re-open if you disagree.

@bear
Copy link
Contributor

bear commented Dec 9, 2014

@mmacaula are you ok with me deleting the branch associated with this PR?

@mmacaula
Copy link
Contributor

mmacaula commented Dec 9, 2014

@bear yeah thanks for asking!

@bear
Copy link
Contributor

bear commented Dec 9, 2014

@mmacaula thanks! looking forward to seeing this land in that other conversation!

@bear bear deleted the silent-collections branch December 9, 2014 22:47
@bear bear removed the triage label Dec 9, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants