Add model changeId event

[paulfalgout]

Fixes #3882
Fixes #4159

Backbone does a bit of extra work to determine when to update _byId on every model change event because change:id will not work if idAttribute has changed. This causes issues as the change event happens after every change: event which means during a change the _byId hasn't updated. Rather than adding complexity to collection the solution is to have the model notify with the id changes.

If adding a public event isn't desired, for an internal solution the model is aware of it's collection and could modify model.collection._byId directly within the set.

Either of these solutions seem preferrable to handling change:[idAttribute]

Replaces the need for:
https://github.com/jashkenas/backbone/pull/4227/files#diff-c773bb9be277f0f3f2baa308b6e0f3a486790fe99fea81ddd0ba409846250571R1205

[jgonggrijp]

Thanks for this insightful fix @paulfalgout. Great to have you still on board.

Backbone does a bit of extra work to determine when to update _byId on every model change event because change:id will not work if idAttribute has changed.

Just a nuance: if I'm understanding the situation correctly, the purpose of the extra work is not to support changes in the idAttribute of individual model classes. The line that I have commented on below illustrates that Backbone does not support such a thing, and supporting it would in fact not be straightforward. Fortunately, the documentation also does not suggest that this is supported.

The real purpose of the extra work is to support polymorphic collections of multiple model types with different idAttributes. Your changes seem just as reasonable under these assumptions.

This causes issues as the change event happens after every change: event which means during a change the _byId hasn't updated. Rather than adding complexity to collection the solution is to have the model notify with the id changes.

Brilliant. I think this is the only correct way to solve the problem. I also like the event name you've chosen; changeId fits in comfortably with change:id and modelId.

If adding a public event isn't desired, for an internal solution the model is aware of it's collection and could modify model.collection._byId directly within the set.

No, this would be an incomplete solution. Each model has only one collection reference, but it might be a member of other collections at the same time. Those other collections would not be notified in this case.

Either of these solutions seem preferrable to handling change:[idAttribute]

change:${idAttribute} does not handle the polymorphic scenario, but it does handle the scenario where a model is a member of multiple collections. So I would say this solution and the previous one are about equally bad.

Replaces the need for: https://github.com/jashkenas/backbone/pull/4227/files#diff-c773bb9be277f0f3f2baa308b6e0f3a486790fe99fea81ddd0ba409846250571R1205

Yes, that's just a combination of two bad solutions.

I would like to request two refinements:

• Include the previous id in the event payload of changeId. I suggest placing it between this and options.
• Add changeId to the Catalog of Built-in Events in the documentation.

Otherwise I'm completely convinced by your test and implementation. 🚀

[jgonggrijp]

This line computes the previous id based on model's current idAttribute.

[paulfalgout]

the purpose of the extra work is not to support changes in the idAttribute of individual model classes

Ah yes, I was being lazy and using idAttribute to refer to whatever attribute is being considered the id rather than the value of idAttribute itself. I think we're on the same page there.

And yep, polymorphic does invalidate the other suggestion. Good catch. I'll update the PR and help in general when I can with changes. I've been lurking a bit. Great to see some movement here.

[paulfalgout]

so regarding:

Include the previous id in the event payload of changeId. I suggest placing it between this and options.

I agree I missed the opportunity to align this with change:id but in that case wouldn't it be the current id that it is changed to?

[jgonggrijp]

so regarding:

Include the previous id in the event payload of changeId. I suggest placing it between this and options.

I agree I missed the opportunity to align this with change:id but in that case wouldn't it be the current id that it is changed to?

Yes that would be more consistent, but then there would be no short way to obtain the previous idAttribute (compare model.id to model.previous(model.idAttribute)). For previous attributes we have previousAttributes(), but there is no corresponding previousProperties() (which would make it entirely consistent), and I don't think adding such a method is warranted.

For what it's worth, I didn't make the remark because of consistency, but because I think event listeners might want to inspect the previous id. Initially, I even thought it might be used in Collection.prototype._onModelEvent, but then I realized that would interfere with the modelId semantics.

[jgonggrijp]

@paulfalgout I intend to make a new release. Would you consider the new changeId event a new feature or mostly just a bugfix? That's going to make the difference between version 1.4.1 and 1.5.0.

[paulfalgout]

Good question.. it's feature-y to me, but not one anyone was specifically asking for. If this is the only feature-like thing, maybe this is a patch release?