Internal _byId index of Collections wrong during model change event

[zzemla]

During model change event, when id attribute changes, the internal _byId index of containing collection is invalid. Because of this, the model cannot be obtained with collection.get(id) method, even though it has ID attribute already set. This is non-intuitive as internal indexing of collections should be transparent to the end user, and when collection.get(ID) returns null, it should be be safe to assume that the model with such ID does not exist in collection.

Internally, this issue is caused by delayed firing of collection's _onModelEvent method which maintains the byId index (

backbone/backbone.js

Line 1144 in 05fde9e

_onModelEvent: function(event, model, collection, options) {

). It is being bound to the all event on model level, and all handlers are invoked only after all other events are processed (

backbone/backbone.js

Line 357 in 05fde9e

if (allEvents) triggerEvents(allEvents, [name].concat(args));

) making the collection update it's index only after change handlers are executed.

Please see the example below:

class Entry extends Backbone.Model
  initialize: =>
    @on 'change', (e) =>
      console.log('onChange Model ', e.id, window.collection.get(e.id), window.collection.get(e.cid), window.collection.get(e.cid).id)

class Collection extends Backbone.Collection
  model: Entry
  initialize: =>
    @on 'change', (e) =>
      console.log('onChange Collection', e.id, window.collection.get(e.id), window.collection.get(e.cid))

window.collection = new Collection()
entry = collection.add({'name':'unsaved'})
entry.set({'name':'saved', 'id':10})

It's console output is:

onChange Model 10 undefined Entry {cid: "c1", …} 10
onChange Collection 10 Entry {cid: "c1", …} Entry {cid: "c1", …}

Please note that in the model change handler the collection is in invalid state, as collection.get(10) produced undefined while collection.get('c1').id returns 10, meaning that the object with ID 10 is actually in the collection.

Also, the second line shows inconsistency with the change event bound at the collection level, which behaves correctly.

[jridgewell]

Why not just use the model passed to the listener? This is almost impossible to fix, because updating the _byId hash is done inside another listener, and we cannot guarantee it is called before yours.

var Collection = Backbone.Collection.extend( {
  model: Entry,
  initialize() {
    this.on('change', (model) => {
      console.log('onChange Collection', model.id, model, model);
    });
  }
});

[zzemla]

I see your point, but sometimes, as in my case, it's not possible to use the model passed to the listener.

On the high level, I think that the way collection's internal indexing is done should be transparent to the user - whether .get implementation is done via a simple array search (i.e. (id) -> _.first( this.models, (m) -> (m.id || m.cid) == id) or via hash-indexing, or any other method, it should not really matter to the user. If object with given ID is in the collection, it should be returned. If not, undefined should be returned. And in neither case a result of .get(ID) should be an object with different ID than the one being searched for (and currently inside model's change callback there is possibility of this).

I know this may be hard to fix with the current implementation, but maybe some warning about this quirk in the docs would help others who may fall into this?

On why it's not always possible to use the model passed to the listener (long read):
In Kanban Tool we have global collections acting as identity map for stored objects. At the same time, we have multiple UI components which have a clean interface for initialization, requiring the objectID on input i.e. <kt-task data-task-id="23" data-user-id="15">, and which upon initialization look for these objects in the global collections. We have encountered this issue when ObjectA was saved, and in the change handler some new UI sub-components were created which were referencing ObjectA by ID - these components could not find it by ID in the global collection, even though it was there.

[jridgewell]

On the high level, I think that the way collection's internal indexing is done should be transparent to the user - whether .get implementation is done via a simple array search (i.e. (id) -> _.first( this.models, (m) -> (m.id || m.cid) == id) or via hash-indexing

We essentially have this due to #2602, which landed in v1.1.0.
Instead of getting by id (collection.get(model.id)), pass in the model (collection.get(model)).