Fetch uses 'parse' on both collection & model, when using {update: true}, it doesn't use parse at the right time #2095

Closed
inlineblock opened this Issue Jan 9, 2013 · 20 comments

Comments

Projects
None yet
5 participants

If I have models that a rooted such as [{model: {id: 1, ...},{model: {id: 2, ...}}] and I use update: true on a fetch, it will never find the models properly since the update method (line 796 in 0.9.9) uses a non-parsed version of the model to determine its ID.

However the fix to this may not be as easy as running the models parse since parse requires an instance.

Also passing in the option of parse defeats the purpose of even including the parse method on the model since it does it directly on the full results, versus each individual, which could cause a duplication of code or an ugly closure using the models parse, but still requires me to pass that parse in each time I want to use update.

Maybe I'm the only one, but my parse methods don't even reference the instance, so maybe it might be a good idea to make it a class method instead and have the 2nd parameter in the parse be the instance, if someone really needs to get to it.

For now, I replaced the line with
model = this.model.prototype.parse ? this.model.prototype.parse(models[i]) : models[i];

Which is ugly since it seems to be an instance type and some people might use parse with references to stuff in the instance.

I'm looking for anyone else to chime in.

I opted to NOT submit a pull request until I get some more feedback.

Contributor

philfreo commented Jan 9, 2013

For what it's worth, my parse methods sometimes do require an instance.

If you say coll.fetch({ update: true, parse: true }) and have a parse method on your Model of return response.model, that doesn't do the trick?

That does not work, since having that as truthy only invokes the collections parse and NOT the models parse.

Since backbone runs both, I have my collection just return back the array and then when it iterates through the array and then passes each object through the models parse, the model does the parsing for only itself.

Basically my collection parse only knows how to get the array of objects and my models parse only knows how to get its data.

Contributor

philfreo commented Jan 9, 2013

Ah gotcha. But if your collection's parse was return _(response).pluck('model'); then it should work, with no changes in Backbone, right? (Though I can understand why you'd want that logic in the Model's parse function instead).

Correct, but my current backend data actually comes more like this:

{models: [{model: {...} , {model: {...}}]}

So my collections parse only does return o.models;
It basically only knows about itself. It shouldn't have to know the definition of the models request, if the model itself takes care of that.

I do see where you are going, however I find that the following statement is no longer fully true (or at least confusing):

After fetching a model or a collection, all defined parse functions will now be run. So fetching a collection and getting back new models could cause both the collection to parse the list, and then each model to be parsed in turn, if you have both functions defined.

Collaborator

braddunbar commented Jan 9, 2013

This is a big problem and also the reason I think we should not be using model data without parsing it normally (e.g. when retrieving models by id/cid). In other words, Collection#get should take model instances as an argument, not unparsed server data.

Using unparsed data breaks a good many assumptions about models being parsed before being used and compared with one another. Data with a namespace wrapper is one example but there are others, including composite keys or otherwise altering the id attribute during parse.

Collaborator

caseywebdev commented Jan 9, 2013

If you need special parsing, you can always...

parse: function (resp, options) {
  var self = this;
  return _.map(resp.models, function (obj) {
    return new self.model(obj, options);
  });
}

Of course this comes with the overhead of creating new models rather than simple raw data, but you should still get the same add/change/remove from update.

Collaborator

braddunbar commented Jan 9, 2013

If you need special parsing, you can always...

I'm not sure what you mean by special parsing. The following is a normal and idiomatic implementation that, to my knowledge, will not currently work with update due to the problems described above.

// model
parse: function(resp) {
  return resp.model;
}

// collection
parse: function(resp) {
  return resp.models;
}
Collaborator

caseywebdev commented Jan 9, 2013

Sorry special was not the right word. Opening a PR with a fix...

caseywebdev added a commit to caseywebdev/backbone that referenced this issue Jan 9, 2013

This issue is currently completely complicating my application flow and I basically have to get around it by tying the view to the model data, checking to see if I've rendered the view and if I already have manually triggering a change event (even if there are no changes).

Obviously this sucks as a work around, but as I don't have the option to use update at all (I require a parse function) there is no other way.

Simply put - it should use parse because there IS a parse option so the programmers has said "hey, this data needs to be parsed before Backbone uses it" whereas right now BB is doing the exact opposite and fundamentally breaking data models in the process.

Collaborator

caseywebdev commented Jan 26, 2013

@abritinthebay I believe this will accomplish what you need

// in your collection
parse: function (res, options) {
  var model = this.model;
  return _.map(res.models, function (data) {
    return new Model(data, options);
  });
}

For me it should read more like this in the collection update method (line 759):

if (options.parse !== false) models = this.parse(models, options);

This fixes everything for me I might add.

(eta - note - I noticed that update doesn't seem to get the options from fetch when I logged it out. This seems odd)

@caseywebdev that won't do a thing because parse is never called in the collection. Ever. At all.

Not when doing a fetch({update:true})

Collaborator

caseywebdev commented Jan 26, 2013

parse is never called in the collection. Ever. At all.

https://github.com/documentcloud/backbone/blob/master/backbone.js#L765
https://github.com/documentcloud/backbone/blob/master/backbone.js#L818

The source says differently =p What version are you using? And is it altered?

0.9.10 and no, it is not altered. I can reproduce it consistently by just switching from.fetch() to .fetch({update:true})

Issue #2193 was where I added more detail but it's incredibly reproducible.

In my testing the options hash is always undefined in the update function so that may have something to do with it. Not that I've changed that code at all.

To clarify - this is all my parse does right now:

parse: function (response) {
    console.log('in collection parse');
    return response.contents;
},

When I use fetch() it's called.

When I used fetch({update: true}) OR update() it's not. In fact the collection basically becomes a complete mess full of recursive junk.

This is with a fresh copy of the development version of BB. In other words - it doesn't work. In fact it's actively detrimental.

Collaborator

caseywebdev commented Jan 26, 2013

There's something else going on in your code. Look at http://jsfiddle.net/bLNtA/1/, it's working as intended.

Strange. I will investigate more.

Collaborator

caseywebdev commented Feb 8, 2013

Fixed with #2251

@caseywebdev caseywebdev closed this Feb 8, 2013

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