[BUGFIX beta] Fix id property in DS.Model once and for all

[acburdine]

No description provided.

[stefanpenner]

in the tests bellow, we only update the ID via:

data/packages/ember-data/lib/system/store.js

Lines 1394 to 1403 in 0f06c86

	updateId: function(internalModel, data) {
	var oldId = internalModel.id;
	var id = coerceId(data.id);
	Ember.assert("An adapter cannot assign a new id to a record that already has an id. " + internalModel + " had id: " + oldId + " and you tried to update it with " + id + ". This likely happened because your server returned data in response to a find or update that had a different id than the one you sent.", oldId === null || id === oldId);
	this.typeMapFor(internalModel.type).idToRecord[id] = internalModel;
	internalModel.setId(id);
	},

which already calls internalModel.setId

What is the point of this observer? As model.get('id') should just be a CP that always reads internalModels id.

Lets remember that the data flows away from the source of truth, in this case the source of truth is InternalModel. And changing the id on a model directly (bypassing the stores public API for this) seems like an anti pattern.

[acburdine]

But what if someone needs to edit the id before saving a new model? That worked before 1.0.0-beta.19

[stefanpenner]

then a getter/setter pair does the trick.

model ... {
  get id()   { return this.internalModel.id }
  set id(id) { this.store.updateId(this.internalModel, id) }
}

internalModel.setId = function(id) {

  if (this.model && this.id !== id) { this.model.notifyPropertyChange(‘id’); } 
}

[acburdine]

if I'm understanding you correctly, that was the first attempt with the whole object.defineproperty thing. That didn't work because of a bug in ember itself.

[stefanpenner]

ug the internal flow here seems very strange.

It really feels like it should be:

model ... {
  get id()   { return this.internalModel.id; }
  set id(id) { this.internalModel.id = id; }
}

internalModel .. {
  set id(newId) {
    var id = this._id,
    if (id === newId) { return; }

    this._id = newId;
    this.store.moveToId(this.type, id, newId);

    if (this.model) {
      this.model.notifyPropertyChange('id');
    }
  },

   get id() {
     return this._id;
   }
}

store ... {
  moveId(type, oldId, newId) {
    let map = this.typeMapFor(type).idToRecord;

    idToRecord[newId] = idToRecord[oldId];
    delete idToRecord[oldId];
  }
}

[acburdine]

That does make sense, but atm it would break when the id property is watched.

[bmac]

@stefanpenner Trying out the code you provided I'm seeing model.id return the correct value however, model.get('id') appears to return a cached undefined value when the id property is watched by the template. I thought notifyPropertyChange would invalidate the cache...

[acburdine]

In the internalModel code, shouldn't this.model actually be this.record?

[stefanpenner]

@bmac on id update, do we not update old IDs in the identify map?

data/packages/ember-data/lib/system/store.js

Line 1400 in 0f06c86

this.typeMapFor(internalModel.type).idToRecord[id] = internalModel;

Otherwise how to we prune the identify map correctly when a model is unloaded.

[bmac]

I don't think a record is slotted into the identity map by id until the id has been acknowledged by the server. This scenario is specifically for updating a record in the new state before it has been slotted into the identity map by id.

[stefanpenner]

strange that unloading a record, transitions it to saved.deleted

[pangratz]

strange that unloading a record, transitions it to saved.deleted

just a quick interception: maybe related #2870

[stefanpenner]

just a quick interception: maybe related #2870

ya looks related, it seems sloppy that we are reusing the same state.

[acburdine]

so.....what should I do to finish this?

[bmac]

@stefanpenner I made another attempt at using id as a ES5 getter/setter. Here are the 2 blockers I've hit so far.

First I tried using the code you provided and I found that this.record.notifyPropertyChange('id'); does not correctly update the _values object on Ember's meta object when id is a watched property. This means doing record.id returns the correct value but record.get('id') returns the old cached value.

Next I tried updating the code so store.updateId calls record.set('id', newId) and allowing the computed setter on id to update the internal model. This correctly updates the cached value in the meta object and both record.id and record.get('id') return the correct value when watched. However the id on internalModel is never updated because Ember ends up defining its own computed setter here that supplants the computed setter defined by ember data.