Send correct 'at' when firing 'add' event #3039

Merged
merged 1 commit into from Sep 11, 2014

Conversation

Projects
None yet
4 participants
Contributor

fastest963 commented Mar 4, 2014

If you add an array of models to a collection and specify at in options when the add event is fired on the collection for each model the at sent in the options is incorrect for that specific model.

The extend is only necessary when the at is not undefined, otherwise we're not modifying it.

Downside of the extend is if someone was wrongly modifying options object in an add event and expecting to get those changes in other events, that is now broken. I had considered changing the original options but that generally doesn't seem to be what any other functions are doing.

Contributor

fastest963 commented Mar 4, 2014

I didn't want to include the overhead of extend in each iteration so I opted against doing:

addOpts = at != null && i > 0 ? _.extend({}, options, {at: at + i}) : options;

inside the loop. Let me know if anyone prefers that instead.

caseywebdev added the change label Mar 6, 2014

Collaborator

caseywebdev commented Mar 7, 2014

Perhaps rather than changing at, you should add an index property. That would ensure this change isn't breaking. We used to always pass index in the event, but indexOf was too expensive for every model. With the at option we get index for free, so I don't see the harm in adding it.

Contributor

fastest963 commented Mar 7, 2014

Won't it seem a little weird if there's a random index property that gets returned only when the at option is passed in? I was favoring updating at only because you passed it in so it would make sense to get it back in the event. The only problem being that the event fires for each model so the at is wrong.

I'm fine with changing it just thought I'd bring that up. Let me know what you think.

Collaborator

caseywebdev commented Mar 7, 2014

Personally, I think it would be weirder to change at than to add an index property. An index would give you the info you're looking for, as well as retain the value of the original at.

Contributor

fastest963 commented Mar 7, 2014

That's correct, but you wouldn't always get an index property. Are we okay with that? Would we want to default at to the collection.length by default then we could provide index in the case where they don't pass that in?

I also just realized that sort is fired BEFORE firing off these events so I should really only provide the index prop when sort is not used, otherwise the data would be incorrect :/

Collaborator

caseywebdev commented Mar 7, 2014

You can't sort when at is set, see https://github.com/jashkenas/backbone/blob/f814b85db65cae8715c67d33606b1352d1373f7f/backbone.js#L701, so that's not an issue.

I don't think providing index when at isn't set is a good idea because it won't be correct. We'll need to get some more input on this, but I think index only being a byproduct of using the at option is not unreasonable.

Contributor

fastest963 commented Mar 7, 2014

Ah missed that, thanks for the clarification. I'll update the PR to pass along index for now and monitor the conversation.

Contributor

fastest963 commented Mar 7, 2014

Also added a test for index being undefined when at is not specified.

Contributor

fastest963 commented Mar 24, 2014

Any updates on this?

@akre54 akre54 and 1 other commented on an outdated diff Mar 24, 2014

@@ -771,8 +771,10 @@
// Unless silenced, it's time to fire all appropriate add/sort events.
if (!options.silent) {
+ var addOpts = at != null ? _.extend({}, options) : options;
@akre54

akre54 Mar 24, 2014

Collaborator

conditionally cloneing seems weird. Just clone and be done with it.

@fastest963

fastest963 Mar 24, 2014

Contributor

If we don't need to clone because we're not modifying anything then why do it? The majority of the time they're not going to passing at so we might as well optimize for that case and not do any unnecessary cloning.

Contributor

fastest963 commented May 6, 2014

I'd like to see this get merged. It's pretty small and all the tests still pass. I commented on @akre54's comment about conditionally cloneing but I don't care too much either way. Let me know if you just want to always clone or if you're fine with the small performance gain vs weirdness.

Collaborator

akre54 commented Sep 11, 2014

@caseywebdev how do you feel about this one?

Collaborator

caseywebdev commented Sep 11, 2014

While it saves an indexOf for handlers needing the position after an add, it worries me a little that index will only be available sometimes (when at is set). Seems kinda gross that an event handler would have to var i = options.index == null ? options.index : this.indexOf(model); to make sure it always has an index. That's my only concern.

Collaborator

akre54 commented Sep 11, 2014

Yeah I see your point (though it'd only be var i = options.index || this.indexOf(model);, just test for the falsey value!). I'm not sure it harms anything by adding it here; your old indexOf checks would still work.

Is there any reason we couldn't always just send the correct index? Like index: this.length + i if at isn't set?

Contributor

fastest963 commented Sep 11, 2014

You can't just check for the falsy value because the index could be 0. We also can't just assume the index is at the end because they might have a comparator which sorts them and so it wouldn't always be at the end.

Collaborator

akre54 commented Sep 11, 2014

Urg. Do we agree it's worth the potential extra check in your handlers? (You'd be doing the indexOf check anyways before.) Or is it ugly enough that we should prefer you run indexOf each time yourself?

Contributor

fastest963 commented Sep 11, 2014

I'd prefer to have the index there and optionally run indexOf than ALWAYS have to run indexOf. Especially if someone adds 500 songs to their queue on Grooveshark ;)

Collaborator

akre54 commented Sep 11, 2014

Alright, I'm with it. Want to rebase and I'll merge?

Contributor

fastest963 commented Sep 11, 2014

Updated. Thanks!

@akre54 akre54 added a commit that referenced this pull request Sep 11, 2014

@akre54 akre54 Merge pull request #3039 from fastest963/master
Send correct 'at' when firing 'add' event
b966584

@akre54 akre54 merged commit b966584 into jashkenas:master Sep 11, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

akre54 referenced this pull request Sep 11, 2014

Merged

Draft changelog for Backbone 1.2.0 #3285

3 of 3 tasks complete

@jashkenas jashkenas added fixed bug and removed change labels Oct 1, 2014

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