Models are now parsed before checking if they exist in a collection

[d-reinhold]

See issue #3760

[jashkenas]

Instantiating models can also have side effects. I don't think we want to create extra ones just for the parse.

[d-reinhold]

The only case where a model is instantiated early and subsequently only used for its parsed attributes is when the model is already in the collection, but the idAttribute is not in the root of the API response. Currently this case is not handled correctly in master: an extra model is instantiated, but instead of merging the new attributes onto the existing model, the existing model is removed from the collection, and the new one is pushed in. This PR fixes that problem, and in all other cases, the model is only instantiated/parsed once, just like before.

Certainly though I could be missing something; if you could provide a failing test case that illustrates your concerns, that'd be really helpful.

[akre54]

This is how we used to do it, actually (#2249 and others). It's unexpected behavior, and a real pain to manage, when you start creating temporary models willy-nilly.

[d-reinhold]

From my perspective, the unexpected behavior is that Collection#fetch doesn't work when your API wraps the idAttribute of your models. This PR fixes that. The code isn't pretty, but neither is Collection#set on master either, as @jashkenas pointed out.

Sure, creating models 'willy-nilly' is bad. This creates a model in a very specific case that is currently unhandled. I guess I'd like to see a concrete example of how things could go wrong if this code was merged?

[akre54]

Sure, and I hear ya. But there's a better way of fixing this than creating temporary models just to get their idAttribute. We can't (and likely shouldn't) enforce that creating a model is idempotent and side-effect-free, regardless of whether we think it's a good idea to put side effects in your initialize or not. It's just too surprising.

Thanks for your feedback and help on this so far. Let's try and get #3758 to a good state that fixes your use case.

[d-reinhold]

@akre54 The parsedModel that is instantiated in this PR is also being instantiated when this code path executes in master. It's not a temporary model that is only used to get the idAttribute. All I'm doing differently is instantiating that model earlier (and explicitly), and using its attributes to update the existing model in the collection (if one exists), or adding the parsed model to the collection (if it wasn't there already). When the preconditions that trigger parsedModel to be instantiated occur in the master branch, the same model is be instantiated here. I guess I did a poor job of explaining that, but this PR is really not doing any extra parsing or instantiating.

I'm all for a clean fix for this in v2; I already left feedback on #3758 last week. But V2 still feels pretty far out. I think this can be fixed in a backwards compatible way, do you disagree?

[akre54]

Line 856 says differently. If {wrapper: {id: 123}} fails the initial get line, it will create a temporary model, then use the temporary model's attributes to find it in the collection.

In your test case, you could use modelId to get the right model from the collection with zero changes to Backbone.

[d-reinhold]

@akre54 Not quite. If {wrapper: {id: 123}} fails in the initial get, a new object parsedModel is instantiated early, correct. If a model with id = 123 is not in the collection, parsedModel is added (and therefore is not being instantiated unnecessarily; it has to be instantiated at some point). If the model with id = 123 is in the collection, we are able to find it, and so we call existing.parse(attrs) to update the existing model, and parsedModel is garbage collected. Seems like a waste.

However, the way Collection#set is implemented in master, the get for {wrapper: {id: 123}} will always fail, even if a model with id = 123 is in the collection. So Backbone will proceed to the add step, and a duplicate model will be instantiated at that point! The existing model with id=123 will get thrown out of the collection, and the duplicate is added instead. What should happen in this situation is the existing model gets updated with the new attributes, if there are any. That's what this PR does.

Do you see why, in both master and in this PR, the same number of models get instantiated in both cases? Obviously we'd like to have no duplicate models created, but this PR doesn't make the situation worse, and it makes the behavior consistent with when the passed-in models are not wrapped.

[d-reinhold]

Regarding Collection#modelId, I guess it could be overridden to return attrs.id || attrs.wrapper.id, but then the collection would have to know about how the model is parsed. I guess that wouldn't be a terrible solution to this problem though.

[akre54]

Yeah it's not the cleanest or clearest solution to have to duplicate logic between parse and modelId (we're thinking of better solutions, would love if you've got one), but I'd argue it's a lot better than creating a model unnecessarily. modelId is the way to go here.