-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Collection#_removeModels is called after 'remove' event #4212
Comments
Thanks @graphographer for reporting and sorry for the late response. I think you are making valid points. Just to make 100% sure that I understand your problem: could you illustrate with some short snippets of code? Thanks in advance. |
Possibly related to #4159. |
Goodness! I haven't worked on my Backbone application since the start of the pandemic. I'll get back to you when I have an extra moment. |
Bump @graphographer. Please let me know if you think you might not get to this any soon; I'll demote the priority of the issue in that case. |
I'm in between projects at work, so I'll take a look and respond with more context. |
@jgonggrijp Here is a fiddle reproducing the unexpected behavior. https://jsfiddle.net/zygomorph/vn6utLb9/46/ |
Thanks, that does clarify the issue. I took a peek at the code and found that half of the code of I also found that #3693 provides some historical background. At the time, the developers didn't realize that the Before I go juggling around code, though: your use case seems a bit exotic. Why would you listen to the |
To be honest, it's been so long I couldn't tell you the use-case I had in
mind at the time I turned this up this issue.
One possibility of the top of my head is to automatically add a removed
model to an "undo" collection.
…On Thu, Jul 20, 2023, 3:52 PM Julian Gonggrijp ***@***.***> wrote:
Thanks, that does clarify the issue.
I took a peek at the code and found that half of the code of
_removeReference is already duplicated before the trigger in _removeModels,
but not the delete model.collection line. This supports your proposal to
move _removeReference before the event trigger (you wrote _removeModels,
but I think you meant _removeReference as the event trigger is *inside*
_removeModels).
I also found that #3693
<#3693> provides some
historical background. At the time, the developers didn't realize that the
model.off line shouldn't be inside the _removeReference method.
Before I go juggling around code, though: your use case seems a bit
exotic. Why would you listen to the remove event in order to insert the
model back into the collection?
—
Reply to this email directly, view it on GitHub
<#4212 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABACEGQEHSKJBP4SCKFAY7DXRGD7ZANCNFSM4HIV4R3Q>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
That would make sense, except that the problem doesn't apply in this case. The problematic line goes if (this === model.collection) delete model.collection; and the condition is no longer true as soon as you add the model to a different collection. So in order for the fix to be necessary, the use case would have to be adding the model back to the same collection. |
@paulfalgout Would be great if you could shine your light on this. |
I think it should be the other way around, as I have a use-case in which I'd like to detect when a model is removed from a collection, manipulate it a bit (such as resetting its id following a destroy) and re-adding it to the collection.
Currently, re-adding a model to a collection during a remove or destroy event seems to lead to unexpected behavior. I think it's in a weird in-between state where it's partway through the process of being disassociated from the collection. So, for example, it won't have a collection property, and I can't add one manually in the event handler.
Using _.defer seems to mitigate all the issues (though this isn't my suggestion for a fix. A fix would just put the event trigger after the _removeModels method call... pretty sure that would work.)
If my use-case doesn't seem to philosophically-opposed to RESTful principles, I might also suggest an option for retaining a model in a collection even when it has been destroyed, say {remove: false}. I am open to suggestions for other patterns for my use-case if this is a totally weird concept, but I won't go into details about my use-case until I've gotten some feedback about this issue.
The text was updated successfully, but these errors were encountered: