Open graphographer opened 5 years ago
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 _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 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?
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 https://github.com/jashkenas/backbone/issues/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 https://github.com/jashkenas/backbone/issues/4212#issuecomment-1644512320, 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.