Trigger error from failing Collection.create with wait:true

[jgonggrijp]

This fixes #4262.

The bug arose because the transaction with the backend is mediated through model.save and the 'error' event is initially triggered by the model. Normally, the collection will forward all events from the model, but with wait: true, the model is not yet added to the collection when the event triggers, so the collection is not listening, either. I addressed this by special-casing the 'error' event in this particular scenario.

I determined that a similar fix is not required for the 'sync' event on successful create. In that case, the model is still added to the collection before the event triggers.

I foresee two possible criticisms on the solution:

1. One could argue that a similar fix is still needed for the 'request' event. However, since the timing of this event is fully predictable (i.e., just before the end of the call to create), users do not need to listen for it in order to match the timing of a callback. Adding special case code for the 'request' event as well would further inflate the code size.
2. It is theoretically possible that a user would call collection.create with a model that is already in the collection. In that case, with wait: true, I expect that the collection will forward the 'error' event twice. If this scenario is considered problematic enough, I am willing to address it.

On the other hand, someone might also argue that this issue does not need fixing at all. The old behavior, while surprising, could be argued to be consistent, since the collection cannot be expected to forward events from a model it does not yet contain.

Reviews and discussion welcome!

CC @paulfalgout @ogonkov @alanhamlett @recursivk

[alanhamlett]

2. It is theoretically possible that a user would call collection.create with a model that is already in the collection. In that case, with wait: true, I expect that the collection will forward the 'error' event twice. If this scenario is considered problematic enough, I am willing to address it.

I think it's better to get an error message rendered twice rather than not at all. Calling collection.add then collection.create is already using the api in an unexpected/unintended way so I'm guessing it's unlikely to be an issue.

[jgonggrijp]

Thanks, both!

I realized something after submitting the PR. Even in the old situation, create would return the new model so you could listen on that for the error event. It can even be a oneliner if you don't need a lasting handle to the model:

someCollection.create({...props}, {wait: true}).on('error', ...);

In other words, listening for the event is already possible, just not on the collection.

With that in mind, do you still consider the change worthwhile?

[alanhamlett]

Also what about view.listenTo(collection, 'error', callback)? Does that already work or also not firing?

[alanhamlett]

With that in mind, do you still consider the change worthwhile?

I think it's worthwhile to make sure the error event triggers as expected, because error cases are often not as tested as success cases so someone might not notice the error even never fires in their code.

[jgonggrijp]

Also what about view.listenTo(collection, 'error', callback)? Does that already work or also not firing?

Not firing. That depends on the collection forwarding the event again.

[Rayraz]

2. It is theoretically possible that a user would call collection.create with a model that is already in the collection. In that case, with wait: true, I expect that the collection will forward the 'error' event twice. If this scenario is considered problematic enough, I am willing to address it.

Imho, sending the error event twice should be considered a bug. If it's not that much of a hassle, I think I would opt to go the extra mile to make sure it only fires once.

If it turns out it will actually be a significat hassle to make sure it only fires once, perhaps that's a sign the status quo is actually more correct than the fix.

Rephrasing the question slightly, if I would have to choose which is less wrong:

1. A collection not forwarding an event for a model that technically isn't part of the collection yet anyway
2. A collection that broadcasts the same error twice

I'd say the first option 1 seems significantly less wrong to me.

[Rayraz]

I can see why you would be hesitant to introduce the this.has(model) check, especially if it becomes significantly slower the more models are in the collection... 🤔 Does the _.noop version negate the this.has(model) check?

If de-duplicating the event is going to be expensive no matter what, perhaps that's a sign that the status quo is a sensible default behavior. I think the argument could be made that having a collection forward events for a model it doesn't actually own is a nice-to-have rather than a must.

If it's an expensive nice-to-have, would it be so terrible to hide this type of event-forwarding behind a feature flag? The 'unexpectedness' of the default behavior could simply be documented.

For people who are already being mindful about performance due to working with large datasets, this is probably just fine:

someCollection.create({...props}, {wait: true}).on('error', callback);

The view.listenTo(collection, 'error', callback) equivalent equally doesn't strike me as completely horrible for someone who's already being very particular for the sake of performance:

view.listenToOnce(someCollection.create({...props}, {wait: true}), 'error', callback);

[jgonggrijp]

I can see why you would be hesitant to introduce the this.has(model) check, especially if it becomes significantly slower the more models are in the collection... 🤔

This is not the case. has is implemented using get, which in turn uses the internal _byId hash table. There is no loop, although it might make a constant number of nested function calls:

https://github.com/jashkenas/backbone/blob/3b37e6aaccbdb8d661487b7cef63e8c930b6f182/backbone.js#L996-L1003

Does the _.noop version negate the this.has(model) check?

No. It only prevents worse bugs. It is cheap, though.

If de-duplicating the event is going to be expensive no matter what, perhaps that's a sign that the status quo is a sensible default behavior. I think the argument could be made that having a collection forward events for a model it doesn't actually own is a nice-to-have rather than a must.

Agreed.

If it's an expensive nice-to-have, would it be so terrible to hide this type of event-forwarding behind a feature flag? The 'unexpectedness' of the default behavior could simply be documented.

I'm open to either documenting the unexpected behavior or silently fixing it (like this PR attempts to do) (although it should be mentioned in the change log). In my opinion, adding features would be too much honor for a corner case like this.

[Rayraz]

The get code doesn't look that bad to me and people probably shouldn't be adding models in bulk this way anyways if they really care about speed? If you'd really like to silently fix this, I would vote to include the this.has(model) fix to avoid double events as well.

If you really don't want to add the this.has(model) check, I'd say double events mean you'd just be replacing one unexpected behavior with another arguably more obscure one. In that case documenting the current behavior would be the better alternative in my opinion.

[jgonggrijp]

I don't have a strong opinion on the this.has(model) check. In order to have one, I would need solid performance measurements, but getting those is a hassle. That would probably not be worth the time.

@alanhamlett have your thoughts on the matter evolved?

[paulfalgout]

TBH the wait kind of makes it less likely that we can know at the point of adding the error if we should attach the listener.. ie: we might not know what the id is to check against until the server returns it.