search

jashkenas / backbone

Give your JS App some Backbone with Models, Views, Collections, and Events
http://backbonejs.org
MIT License
28.1k stars 5.39k forks source link

Add model changeId event #4249

Closed paulfalgout closed 2 years ago

paulfalgout commented 2 years ago

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

paulfalgout commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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?