Back to the original {add:true} on fetch #2048

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
9 participants
Collaborator

tgriesser commented Dec 29, 2012

There has been a little bit of confusion around how add:true should work with the addition of the update API (#2008, #1975, #2009).

The {add:true} is a common enough case that it should also prevent the items from being removed - rather than needing to specify {update:true, remove:false}.

This pull request gives collection.fetch({add:true}) the same implementation it had before, without affecting the update api call, with the ability to specify additional options such as {merge:false} if desired.

Collaborator

caseywebdev commented Dec 29, 2012

I think this is a step backwards in adjusting people's mindsets. The idea behind update is that you're matching your local data to the response data. When you want to do something that disrupts this harmony, it should be specified. For example I won't want the response to remove any local data, {remove: false}. This is also why I like update as the default action for fetch. This allows you to only have to specify one option for most actions. {reset: true}, {remove: false}, {add: false}, etc...

Collaborator

tbranyen commented Dec 29, 2012

I agree, I feel like this is a documentation problem and not an implementation problem when it comes to confusion.

Collaborator

tgriesser commented Dec 29, 2012

I guess making update the default would help, it currently feels like a step backward to go from needing one parameter .fetch({add:true}) for the most common use in something like an infinite scroll, to two: .fetch({update:true, remove:false}).

Collaborator

akre54 commented Dec 30, 2012

@caseywebdev: I get the semantics you're going for—and in a large number of use cases you'd want to only update the models you already have in your collection (high scores, friends lists, etc)—but I have to agree with @tgriesser that the new fetch options are overly confusing for what is clearly a very common use case for Collections: appending models to your existing client-side collection without removing or merging what you already have.

@tgriesser's patch still feels a little hackish (conditionally extending options in fetch based on add option, only to extend it again in update), but I like the API it exposes much more than the alternative.

Collaborator

akre54 commented Dec 30, 2012

I vote in favor of setting fetch default to update instead of reset, since usually when I fetch a collection I don't want to reset it completely. That way, my fetch can handle both initial data loading and the "fetch more" case using the same method:

// in a Collection of discussions, e.g.
fetch: function(options) {
  options = _.extend({
    add: true,
    data: {
      offset: this.length
    }
  }, options);
  return Collection.prototype.fetch.call(this, options);
}

... 

// in my controller logic:
var c = new DiscussionsCollection;
c.fetch(); // initial bootstrapping

// in a fetchMore event:
c.fetch(); // adds more models to collection

Similarly, if I have a collection that doesn't need appended models, I don't even have to override fetch:

 // in controller logic
var c = new HighScoresCollection;
c.fetch(); // initial boostrapping

// update collection every 10 seconds
// will replace and deep-merge current collection models with response
setTimeout(c.fetch, 10000);

+1 for defaulting to update

why not fetch? update is better?

2013/1/2 Dirk Bonhomme notifications@github.com

+1 for defaulting to update


Reply to this email directly or view it on GitHubhttps://github.com/documentcloud/backbone/pull/2048#issuecomment-11801832.

Collaborator

caseywebdev commented Jan 2, 2013

@stevezheng Right now fetch internally calls reset (by default) to add new models. I propose it defaults to update instead.

Contributor

gsamokovarov commented Jan 2, 2013

I'm +1 for the default update. Its usually what I want, anyway.

Owner

jashkenas commented Mar 19, 2013

set is now the default -- problem solved.

@jashkenas jashkenas closed this Mar 19, 2013

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