Embedded records mixin should use the correct serialization key when deserialize configuration is set, Fixes #2556

[agrobbin]

This is my first attempted contribution to Ember Data, so if I'm approaching anything the wrong way, please let me know!

When extending the DS.EmbeddedRecordsMixin, if you configure a belongsTo relationship as { serialize: 'id', deserialize: 'records' }, it won't actually pay attention to the serialize: 'id' option, but use the deserialize: 'records' option. This is because of serializeBelongsTo and the reimplementation of keyForRelationship, which notices the deserialize option instead of the serialize option.

Fixes #2556.

[joeruello]

Hit this today, thanks for the fix!

[bmac]

I think the convention is to put the conditional body on its own line so its easier to inset a breakpoint if needed,

[bmac]

For symmetry, I'd like to see the 'deserialize' argument included on the keyForRelationship relationship call at line https://github.com/agrobbin/ember-data/blob/2556-embedded-records-mixin-serialize-ids/packages/ember-data/lib/serializers/embedded_records_mixin.js#L340

[bmac]

@pixelhandler and @igorT would one of you like to review this pr since you have more familiarity with the EmbeddedMixin and (presumably) with ActiveRecord?

[pixelhandler]

@bmac I can take a look in the next day or two

[agrobbin]

@bmac done and done!

[igorT]

This isn't needed anymore right?

[igorT]

Thanks for the PR! Looks pretty good. If we are gonna add 'serialize'/'deserialize', could you do it throughout all the serializers and not just the EmbeddedRecords one?

[agrobbin]

@igorT I can certainly do that, but won't that cause jsHint warnings about unused arguments in the keyForRelationship function definitions when it happens to not be used?

[agrobbin]

@igorT or were you thinking of just adding the argument to the call-sites and since we only happen to use it in the EmbeddedRecordsMixin mixin, leave the other keyForRelationship definitions alone?

[agrobbin]

@igorT I went with the latter. Let me know if I can do anything else!

[agrobbin]

@igorT is there anything remaining to get this in before beta 16 goes out?

[bmac]

@agrobbin This issue was discussed in the last Ember Data meeting and it was decided that if this change is made for keyForRelationship the method param should also be passed into keyForAttribute as well.

[agrobbin]

@bmac 👍 just pushed up a second commit to this branch, happy to squash them into 1 assuming you like how this looks!

[pixelhandler]

Seems good 👍

To support getting the right key, it seems necessary to know whether the key will be used to serialize or deserialize, nice.

[agrobbin]

Squashed!

[igorT]

Thank you @agrobbin

[agrobbin]

No problem!