Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use model's serializer "transformFor" method #5

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mpirio
Copy link
Contributor

@mpirio mpirio commented Aug 17, 2017

The addon need to use model's serializer "transformFor" method to support all transform's types (like in ember-data-model-fragments addon).

mpirio added 3 commits August 17, 2017 16:43
- to support all transform's types (like in ember-data-model-fragments addon)
@offirgolan
Copy link
Owner

@mpirio serializerFor and transformFor are both private methods. In what Ember data versions were they introduced?

@mpirio
Copy link
Contributor Author

mpirio commented Sep 5, 2017

@offirgolan store#serializerFor is a public method but JSONSerializer#transformFor is a private method. I proposed to use this private method because ember-data-model-fragment redefine it.

@Techn1x
Copy link

Techn1x commented Oct 25, 2017

I am also using ember-data-model-fragments with ember-data-copyable

I tried making the changes in this PR to my code for testing, and the transform is returned from getTransform successfully, but I end up with a different error on the next line when the fragment itself is getting serialized;
https://github.com/offirgolan/ember-data-copyable/blob/master/addon/mixins/copyable.js#L149-L154

transform.serialize(value, attributeOptions) calls serializeFragment(snapshot) in ember-data-model-fragments, but fails on this line because snapshot.modelName is undefined
https://github.com/lytics/ember-data-model-fragments/blob/master/addon/transforms/fragment.js#L37

I am not sure if this is a problem with this addon or ember-data-model-fragments...

Thoughts?

@offirgolan
Copy link
Owner

@mpirio I think we should have a fallback if transformFor is not available. I'm not sure what version of ember-data it was introduced so I'm not too comfortable shipping this without falling back to the previous behavior.

@mpirio
Copy link
Contributor Author

mpirio commented Nov 2, 2017

@Techn1x I don't know if it's a problem in "ember-data-copyable" or in "ember-data-model-fragments" ... Have you tried the latest version 0.2.2: if you use fragments, this version uses the Ember.Copyable copy() method defined in fragments.

@mpirio
Copy link
Contributor Author

mpirio commented Nov 2, 2017

@offirgolan Yes you're right. I look at this :)

@Techn1x
Copy link

Techn1x commented Nov 2, 2017

@mpirio Thanks a lot! That works well now!

Unfortunately I also have yet another (somewhat unrelated) issue. Some of my model attributes are moment objects, and moment objects don't implement Ember.Copyable (looks like you've already seen it though ;) )
jasonmit/ember-cli-moment-shim#148

@mpirio
Copy link
Contributor Author

mpirio commented Nov 2, 2017

@Techn1x Great :)

About your problem with "moment" attributes, i have a solution and I will soon doing a PR. I do not know if it's the best solution but it works in my projects.

@offirgolan offirgolan mentioned this pull request Dec 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants