jashkenas / backbone

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

Collection does not purge internal `_byId` reference to a removed model #4159

Closed cueedee closed 2 years ago

cueedee commented 7 years ago

Please find below about the tersest reproduction I could fathom of a sequence of steps in a Backbone application that leads to a Collection getting into a state of internal confusion.

A working copy can be found in this JSFiddle .

What it demonstrates is a collection keeping an internal (_byId) reference to a remove()-ed Model. That Model is no longer part of collection.models, however, it can still be retrieved through its (former) id.

var
    MyView = Backbone.View.extend( {

        initialize: function () {

            this.model = new Backbone.Model( {

                id:  'foo'
            ,   foo: 'bar'
            } );

            this.collection = new Backbone.Collection( this.model );

            this.listenTo( this.model, 'change:id', function ( model, id ) {

                if ( id == null ) {
                    this.collection.remove( model );
                }

            } );

            this.model.unset( 'id' );

            $('#models' ).val( JSON.stringify( this.collection.models      ));
            $('#_byId'  ).val( JSON.stringify( this.collection._byId       ));
            $('#get-foo').val( JSON.stringify( this.collection.get( 'foo' )));
        }

    })

, foo = new MyView()
;
TM-Olashyn-Dmytro commented 7 years ago

Hmm, interesting. When we add model to collection, _byId add two properties: model.id and model.cid, their reference to model. Issue on 1134 line, id === undefined, but _byId contains model.id - 'foo'.

Collection don't has some special logic when change model.id to value == null, check line 1183.

Also you can change unset to set null or undefined. this.model.set('id', null);

P.S.: I think you never need set your model id to null or undefined, it is anti pattern.

backbone_remove

backbone_model_event

jashkenas commented 7 years ago

Yes — you should never do this, but it's still a bug.

We should fix this by making sure that if an id is changing from a value to null, we delete this._byId[id] first.

rgroothuijsen commented 5 years ago

If you remove a model from a collection without the use of an event, it works as expected. The problem is in _onModelEvent which checks if the name of the event is change, when the actual event name being sent in this case is change:id.

jgonggrijp commented 2 years ago

This is almost certainly a duplicate of #3882, but I'll keep both tickets open for now, because both contain part of the symptoms and part of the explanation.