Skip to content

Loading…

Fix #2068 - `set` respects input order in absence of comparator #2085

Merged
merged 1 commit into from

9 participants

@caseywebdev
Collaborator

This patch would allow collections to be reordered (without destroying any existing models). This is useful when response data can only be sorted server-side.

@braddunbar
Collaborator

Hi @caseywebdev! So the motivation for this is to take models that already exist in the collection and ensure that they're appended to the end of the list along with the other updated models? In what case is this needed? Collection#add has grown quite a few moving parts and options of late and I'm not sure that we need another one in this case.

@caseywebdev
Collaborator

It's for the case in #2068 where a client-side ordering is not possible and the order the server responds with needs to be reflected in the local collection, but without destroying existing models.

Hopefully @shadel and @philfreo who expressed desire for this functionality will chime in with specific use cases.

@philfreo

I have no immediate need for this, but it seems reasonable. An example would be a collaborative todo list editor that polls todos.fetch({ update: true }) in the background, where the order of the todo items is based on their order in the api response. As things change on the server, you already can get add/remove/change events - the only thing missing would be a sort or reorder event.

@caseywebdev
Collaborator

@philfreo You already get the sort event in master after relevent updates. This patch is only relevant for collections that cannot be sorted client-side.

@philfreo

I was referring to if the server responds with a list of models where the server's order is what matters. It's not sortable based on some model attribute.

@caseywebdev
Collaborator

Ah, well yes in that case you'll only get correct ordering (and the sort trigger) with this patch or something similar.

@braddunbar
Collaborator

This patch is only relevant for collections that cannot be sorted client-side.

In what situation is this the case, and how does this patch help? It appears to remove existing elements and then re-insert them at the end of the collection but still leave the rest of the models as they were. Is that correct?

@caseywebdev
Collaborator

It'd really be used for update more than add, i.e.:

var collection = new Collection([{id: 3}, {id: 1}]);
collection.fetch({
  update: true,
  reorder: true,
  success: function () {
    // let's say the response is [{id: 1}, {id: 2}, {id: 3}]
    collection.pluck('id'); // in master: [3, 1, 2], with patch: [1, 2, 3]
  }
});

I'll look at an alternate implementation as only an update option.

@tgriesser
Collaborator

Let's say the models are presorted from a query on the server:

model.fetch({update:true, data: {name:'asc', type:'winners', page:3}});

It's unintuitive that the models coming back from the server aren't in the order that they're requested, and while you can define client side comparators for this, that still doesn't seem like the best move. I guess in this case, reset should be used rather than update - but if people are pushing for update as the default (in #2083) then it's confusing that models aren't automatically kept in the order they're sent from the server.

@philfreo

Although I guess in this case, reset should be used rather than update

You may not want it to completely recreate every model in the collection (nor would you want to redraw DOM elements for each existing model). You might even want to animate the reordering of existing views to match the newly updated collection order (based on the order returned in the api response)

@braddunbar
Collaborator

@caseywebdev As @tgriesser mentions above, reset works perfectly in that case, with much less ceremony (no options required).

var collection = new Collection([{id: 3}, {id: 1}]);
// Response: [{id: 1}, {id: 2}, {id: 3}]
collection.fetch({
  success: function () {
    collection.pluck('id'); // [1, 2, 3]
  }
});
@caseywebdev
Collaborator

See @philfreo's comment. Any hooks you had with your existing models are obliterated with reset.

@philfreo

I can't see a need for reorder outside of update though.

@caseywebdev
Collaborator

@philfreo I agree, I shouldn't have advertised it as an add option, but rather as an update option. I've been toying around and this is still the best implementation I've found.

@braddunbar
Collaborator

Any hooks you had with your existing models are obliterated with reset.

@caseywebdev You're right, the hooks will be removed. I've gotten around this in the past by providing a factory function to Collection#model that looks up existing models instead of returning new ones (very effective in my experience).

I understand your motivation for that type of functionality, but I don't think we should break other functionality to accomplish it (see my comments on #2095). Also, please forgive me for being a bit tangential as this doesn't directly inform the solution to the issue in question. :)

I can't see a need for reorder outside of update though.

Yep, if we're going to add this let's not complicate add if there is no added value outside of update.

@caseywebdev
Collaborator

Yep, if we're going to add this let's not complicate add if there is no added value outside of update.

@braddunbar I agree, my latest implementation stays out of add completely.

@caseywebdev
Collaborator

Here's another use...

cards.update(cards.shuffle(), {reorder: true});

Doing this on master is much more involved (unless you're reseting of course, but we're assuming we want the same models).

@wyuenho

I wonder if reorder should default to true. Are there a lot of cases where you don't want the client side collection to reflect the order from the server?

@caseywebdev
Collaborator

@wyuenho If you offload sorting to the client side, there's no need to send sorted data in the response. Also, I wouldn't default reorder as it carries more overhead than a standard add/merge.

@philfreo

+1 for this now that its implementation is in Collection#update

@mateusmaso

+1 for this also, update = reset + smart

This was referenced
@jashkenas
Owner

I'm afraid I'm not yet persuaded that we need this ... or that if we do need this, it needs to be an option. Either it should be how Collection#set behaves ... or it should be up to you. More persuasive real-world use cases where it wouldn't be cleaner to do the sorting on the client-side (even if the server is simply providing a sortOrder property in the model) would help make your case.

@jashkenas jashkenas closed this
@tgriesser tgriesser reopened this
@tgriesser
Collaborator

So as @molily pointed out on #2443 - now that set (formerly update) is the default fetch handler, I think this is worth revisiting, as there are definite benefits to maintaining sort order from the server side.

@jashkenas
Owner

Agreed. But would you get a "change" event, for a change in position of a model that already exists? No notification of the move? It seems like the semantics here are a bit sticky.

@molily

Good point.

collection.set([ {id: 0}, {id: 1}, {id: 2} ]);
collection.set([ {id: 0}, {id: 2}, {id: 1} ]);

If this would change the order to 0-2-1, this would need to fire some event. Otherwise there is no way to update the UI. It should at least be a simple update event so a CollectionView can check if the DOM order matches the collection order. This PR was using sort for this purpose.

@caseywebdev
Collaborator

Well in this PR, the sort event would fire whenever the order or cardinality of the collection changes. That seems like a logical event to me that's already in the core.

@wookiehangover
Collaborator

To @caseywebdev's point, it seems like a sort event is an appropriate consequence of { reorder: true }

@caseywebdev
Collaborator

Just updated this branch to work with BB 1.0.0.

NOTE: This new implementation doesn't introduce any new options. The decision to reorder the collection or not is based on !sortable && options.add && options.merge && options.remove. This means you can pass the existing {sort: false} if you do have a comparator and want the collection to be ordered according to the input. A rare case I'm sure and more likely just a convenient, logical side effect.

@wookiehangover wookiehangover commented on an outdated diff
backbone.js
((6 lines not shown))
var at = options.at;
var sortable = this.comparator && (at == null) && options.sort !== false;
var sortAttr = _.isString(this.comparator) ? this.comparator : null;
var toAdd = [], toRemove = [], modelMap = {};
+ order = !sortable && options.add && options.merge && options.remove && [];
@wookiehangover Collaborator

Just curious, why the && []?

@caseywebdev Collaborator

order doubles as false when reordering won't be considered and acts as the array that stores the _prepareModel'd models when reordering is going to be done.

should put the var on this line keeping the pattern of the previous 4 lines

@caseywebdev Collaborator

I had it there, but it pushes the line over 80 which makes me sad. I think we can store local references to options.add and options.merge since they're accessed in the loop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jashkenas
Owner
  • Lines over 80 aren't a big deal. If that's how many variables we have, so be it.

@braddunbar -- what do you think about the current state of this PR?

@braddunbar braddunbar commented on an outdated diff
backbone.js
@@ -666,6 +666,8 @@
var sortable = this.comparator && (at == null) && options.sort !== false;
var sortAttr = _.isString(this.comparator) ? this.comparator : null;
var toAdd = [], toRemove = [], modelMap = {};
+ var add = options.add, merge = options.merge, remove = options.remove;
+ var order = !sortable && add && merge && remove && [];
@braddunbar Collaborator

This is a bit too clever for my taste. How about a ternary instead?

var order = !sortable && add && merge && remove ? [] : false;
@caseywebdev Collaborator

De-clevered.

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

I haven't run into this issue in real code so I don't have a strong opinion about its inclusion. However, I much prefer this version to the one that adds yet another option to the API.

@jashkenas
Owner

Before merging, can we get some more tests in this PR?

Specifically, to be certain that the behavior is sane when using different combinations of the add, merge, and remove flags ... and different combinations of models that are-already or are-not-already in the collection.

@caseywebdev
Collaborator

@jashkenas I added a few more cases. Obviously there are a bunch of possible permutations here, so if there are some specific cases you want in the tests let me know.

@jashkenas
Owner

Looks ok to me -- merging. If anyone wants to think through the desired consequences and/or contribute any more permutations to the test, that would be lovely.

@jashkenas jashkenas merged commit 165849e into jashkenas:master

1 check passed

Details default The Travis build passed
@caseywebdev caseywebdev deleted the caseywebdev:reorder branch
@philfreo

I'm running into an issue related to this, where I expect Collection#set to respect input order even if there's a comparator, when sort: false is passed.

I've added some permutations to the tests regarding sort: false together with remove: false: https://github.com/philfreo/backbone/blob/order/test/collection.js#L1036-L1062
All the assertions pass, except one fails.

@caseywebdev do you agree this test should pass? if so, thoughts on a fix?

@caseywebdev
Collaborator

In my opinion, the current functionality makes the most sense. I don't think you should be able to reorder with remove: false because the results would be ambiguous. Should the group you passed in be set at the front of the collection with models that were in the collection in the back? The opposite? Should existing model positions be respected and just new models ordered in the back? The only way to definitively order/re-order is to match the collection order to exactly what was passed in, no more and no less.

@philfreo

Fair enough, there's definitely some ambiguity (I partially accounted for this in my test).

My use case was: fetching/updating the latest 25 items in a server-side paginated collection. I want to show the items in the UI in the same order where they are on the server, and they get added to the UI upon add events. Normally, the items appear in the correct order.

I need to use {remove: false} because other "pages" of the collection may have been loaded via infini-scroll, and I don't want those models to disappear when refreshing the 1st page. The UI simply responds to add and change events.

But in the case where a new item is returned at the beginning of the list, from the server, due to this issue Backbone is sticking it at the end (instead of the beginning, like in the server response). So then the UI shows this new item at the end, instead of the beginning :(

@caseywebdev
Collaborator

You can force the reorder by removing the remove: true option and using

parse: function (data) {
  // New data first, or however you want to order the new list relative to the old
  return data.concat(this.models); 
}
@philfreo

(You mean be removing remove: false)

Really cool idea, thanks.

@caseywebdev
Collaborator

Yup, that's what I meant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Showing with 30 additions and 7 deletions.
  1. +11 −7 backbone.js
  2. +19 −0 test/collection.js
View
18 backbone.js
@@ -666,6 +666,8 @@
var sortable = this.comparator && (at == null) && options.sort !== false;
var sortAttr = _.isString(this.comparator) ? this.comparator : null;
var toAdd = [], toRemove = [], modelMap = {};
+ var add = options.add, merge = options.merge, remove = options.remove;
+ var order = !sortable && add && remove ? [] : false;
// Turn bare objects into model references, and prevent invalid models
// from being added.
@@ -675,14 +677,14 @@
// If a duplicate is found, prevent it from being added and
// optionally merge it into the existing model.
if (existing = this.get(model)) {
- if (options.remove) modelMap[existing.cid] = true;
- if (options.merge) {
+ if (remove) modelMap[existing.cid] = true;
+ if (merge) {
existing.set(model.attributes, options);
if (sortable && !sort && existing.hasChanged(sortAttr)) sort = true;
}
// This is a new model, push it to the `toAdd` list.
- } else if (options.add) {
+ } else if (add) {
toAdd.push(model);
// Listen to added models' events, and index models for lookup by
@@ -691,10 +693,11 @@
this._byId[model.cid] = model;
if (model.id != null) this._byId[model.id] = model;
}
+ if (order) order.push(existing || model);
}
// Remove nonexistent models if appropriate.
- if (options.remove) {
+ if (remove) {
for (i = 0, l = this.length; i < l; ++i) {
if (!modelMap[(model = this.models[i]).cid]) toRemove.push(model);
}
@@ -702,13 +705,14 @@
}
// See if sorting is needed, update `length` and splice in new models.
- if (toAdd.length) {
+ if (toAdd.length || (order && order.length)) {
if (sortable) sort = true;
this.length += toAdd.length;
if (at != null) {
splice.apply(this.models, [at, 0].concat(toAdd));
} else {
- push.apply(this.models, toAdd);
+ if (order) this.models.length = 0;
+ push.apply(this.models, order || toAdd);
}
}
@@ -723,7 +727,7 @@
}
// Trigger `sort` if the collection was sorted.
- if (sort) this.trigger('sort', this, options);
+ if (sort || (order && order.length)) this.trigger('sort', this, options);
return this;
},
View
19 test/collection.js
@@ -996,6 +996,25 @@ $(document).ready(function() {
collection.set({}, {parse: true});
});
+ test('`set` matches input order in the absence of a comparator', function () {
+ var one = new Backbone.Model({id: 1});
+ var two = new Backbone.Model({id: 2});
+ var three = new Backbone.Model({id: 3});
+ var collection = new Backbone.Collection([one, two, three]);
+ collection.set([{id: 3}, {id: 2}, {id: 1}]);
+ deepEqual(collection.models, [three, two, one]);
+ collection.set([{id: 1}, {id: 2}]);
+ deepEqual(collection.models, [one, two]);
+ collection.set([two, three, one]);
+ deepEqual(collection.models, [two, three, one]);
+ collection.set([{id: 1}, {id: 2}], {remove: false});
+ deepEqual(collection.models, [two, three, one]);
+ collection.set([{id: 1}, {id: 2}, {id: 3}], {merge: false});
+ deepEqual(collection.models, [one, two, three]);
+ collection.set([three, two, one, {id: 4}], {add: false});
+ deepEqual(collection.models, [one, two, three]);
+ });
+
test("#1894 - Push should not trigger a sort", 0, function() {
var Collection = Backbone.Collection.extend({
comparator: 'id',
Something went wrong with that request. Please try again.