allowing to pass an has many relationship on create record. Fixes #1497 #2400
Conversation
sly7-7
changed the title
| @@ -248,6 +248,7 @@ Store = Ember.Object.extend({ | |||
| createRecord: function(typeName, inputProperties) { | |||
| var type = this.modelFor(typeName); | |||
| var properties = copy(inputProperties) || {}; | |||
There was a problem hiding this comment.
I would call _extractAttributes otherwise we will set the relationship twice. Maybe extract attributes, relationships and then warn if there is anything left?
|
Is this ready? Can you add some tests for the case where we have bunch of attrs and relationships together, to guard the case we discussed earlier. |
|
@igorT Sure, is https://github.com/emberjs/data/blob/master/packages/ember-data/tests/unit/store/create_record_test.js a good place to write them ? |
|
Comments would be nice, your call |
|
|
||
| // Set the properties specified on the record. | ||
| record.setProperties(attributes); | ||
| record._preloadData(relationships); |
There was a problem hiding this comment.
Actually preloadData handles attributes as well, no?
There was a problem hiding this comment.
Yeah, I was thinking about that, but I'm not sure it has the same result as setProperties. For example I'm not sure that https://github.com/emberjs/data/blob/master/packages/ember-data/lib/system/model/model.js#L720 will trigger properties change. You should know more than me about that.
There was a problem hiding this comment.
Good point. I don't think we should use it for relationships as well actually.
There was a problem hiding this comment.
Great catch actually, we shouldn't use it for relationships at all. The ED semantics are different, preloading means that data exists on server, if you createRecord it doesn't
There was a problem hiding this comment.
Hum, actually when I read that: https://github.com/emberjs/data/blob/master/packages/ember-data/lib/system/model/model.js#L742, I thought it was exactly what we were looking for. So I was using preloadData for relationships only to use that path. (Also, it seemed to me that makes createRecord support passing relationships as ids as well).
There was a problem hiding this comment.
So for now the behavior happens to end up the same because we don't do relationship change tracking, but once we do, we want preloading to not dirty while passing on create should
|
Ok, I will do my best to do it this evening/night (busy until about 11pm GMT) |
|
Thank you! |
|
Obviously, I think this needs more tests:
|
| return relationship.getRecords(); | ||
| }).meta(meta).readOnly(); | ||
| }).meta(meta); |
There was a problem hiding this comment.
seems like it could backfire on us. @igorT thoughts to making this not readOnly?
There was a problem hiding this comment.
model.set('posts', [post1, post2[) should work though?
|
@igorT One more ping for you ;) |
|
+1 |
|
@igorT rebased, updated using relationship.clear() and computedPolyfill. Though I don't know what to do with #2400 (comment) |
|
Your rebase seems off by 1, it seems to have pulled another commit with it. |
|
Otherwise looks good, ready to merge |
|
@igorT Thank for the review, do you want me to merge that after the right rebase ? |
|
as long as travis passes |
Using record's setProperties and making hasMany rel not read only anymore
|
Ok, I don't know what was going on, but I had a commit about the beta.18 beeing released. This should be good now |
allowing to pass an has many relationship on create record. Fixes #1497
|
Thanks! |
Fixes #1497
cc/ @igorT