Use Ember.Copyable mixin features if present #6
Conversation
|
@offirgolan Any response? Do not hesitate to contact me if you need further informations, etc. Thank you. |
addon/mixins/copyable.js
Outdated
| // a new instance. | ||
| value = transform.serialize(value, attributeOptions); | ||
| value = transform.deserialize(value, attributeOptions); | ||
| if ( |
There was a problem hiding this comment.
This can just be
if (canInvoke(value, 'copy')) { ... }
There was a problem hiding this comment.
Yes, we can use Ember.canInvoke. But i would like to be sure value is an Ember Object which extends Ember.Copyable mixin and so, value#copy() is really the method that copy this object (and not a method to do anything else). I do not know how to do... Perhaps do you know?
There was a problem hiding this comment.
@offirgolan Any response ? I will make the change but Ember.canInvoke is a private method. We should use Ember.tryInvoke method.
addon/mixins/copyable.js
Outdated
| (Ember.typeOf(value) === 'instance') && | ||
| (Ember.typeOf(value.copy) === 'function') | ||
| ) { | ||
| value = value.copy(deep); |
There was a problem hiding this comment.
Please add a comment here as to how we can get to this point.
|
@offirgolan Hello. I made the changes. Is it OK for you? Thanks. |
|
@mpirio thanks for taking the time to put this together! 😸 |
Hello,
I use ember-data-model-fragments and I had a problem when using ember-data-copyable (ember-data-model-fragments use own type but this could be the same for other addons) so i sent you this first PR.
But this first PR is not enough to support ember-data-model-fragments. A fragment extends DS.Model and when ember-data-copyable (de)serialize to get a new value instance, it's should be a DS.Snapshot and not a DS.Model instance which should be passed as argument.
I think it should be better to use
copymethod (cf Ember.Copyable) ifvalueis an Ember Object instance and if it use Ember.Copyable mixin. So this new PR.What do you think about this proposal? Thanks.