[BUGFIX release] fix regression where dynamically set id is not serialized in a create… #3701
Conversation
acburdine
changed the title
| @@ -295,7 +295,7 @@ var Model = Ember.Object.extend(Ember.Evented, { | |||
| @property id | |||
| @type {String} | |||
| */ | |||
| id: null, | |||
| id: Ember.computed.alias('_internalModel.id'), | |||
There was a problem hiding this comment.
I'm worried about this being a breaking changed. It is possible to access a record's id without using a getter (record.id). Maybe we want to use an ES5 getter to grab the id out of the internal model?
There was a problem hiding this comment.
Would that work for setting the id as well? The main cause of the regression is that the internalModel id is not being set correctly when it is modified after creation.
There was a problem hiding this comment.
I think it would work for setting the id as well if we also implemented an ES5 property setter.
|
Won't this mess up the typeMaps in store? I know there are issues related to this and it would be great to get it fixed, just want to make sure we're solving it in a reliable way. Ping @igorT for review. |
|
@wecc I believe the store already has code to update the typeMaps after the |
|
@bmac Anything else I need to do for this or is it just waiting on review? |
| @@ -541,9 +540,8 @@ InternalModel.prototype = { | |||
| }, | |||
|
|
|||
| setId: function(id) { | |||
| Ember.assert('Object is a newly created model instance', this.isNew); | |||
There was a problem hiding this comment.
isNew is a function. Ember.assert('Object is a newly created model instance', this.isNew());
There was a problem hiding this comment.
Also lets make the assertion message say something like "A record's id can not be chanced once it is in the loaded state."
acburdine
force-pushed
the
id-fic
branch
2 times, most recently
from
8c99b0b
to
385224e
Compare
| @@ -541,9 +540,8 @@ InternalModel.prototype = { | |||
| }, | |||
|
|
|||
| setId: function(id) { | |||
| Ember.assert('A record\'s id cannot be changed once it is in the loaded state', this.isNew()); | |||
There was a problem hiding this comment.
Looks like there are a few code paths that try to set a record with an id to the same id. Changing the assertion logic to the following should work.
var oldId = this.id;
Ember.assert('A record\'s id cannot be changed once it is in the loaded state', oldId === null || id === oldId || this.isNew());
acburdine
force-pushed
the
id-fic
branch
2 times, most recently
from
07485f2
to
f27685e
Compare
| return this._internalModel.id; | ||
| } | ||
| } | ||
| }) |
|
@acburdine Looks like these changes have caused this test to fail. I think the simplest fix would be to add this assertion to the end of the model's willMergeMixin method. Ember.assert("You may not set `id` as an attribute on your model. Please remove any lines that look like: `id: DS.attr('<type>')` from " + constructor.toString(), Object.keys(props).indexOf('id') === -1); |
|
ping @igorT I'd like you to review this pr if you have time. |
… save request closes emberjs#3694 - uses es5 getters and setters to be able to set the internalModel id - add assertion that model is new before setting id
| } | ||
| }, | ||
| get() { | ||
| if (this._internalModel) { |
[BUGFIX release] fix regression where dynamically set id is not serialized in a create…
… save request
closes #3694