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

A proper migration from 0.8.x guide #2082

Closed
phuongnd08 opened this issue Mar 21, 2017 · 12 comments
Closed

A proper migration from 0.8.x guide #2082

phuongnd08 opened this issue Mar 21, 2017 · 12 comments

Comments

@phuongnd08
Copy link

I'm reading this: https://github.com/rails-api/active_model_serializers/blob/master/docs/howto/upgrade_from_0_8_to_0_10.md

When I read through to this point: "Add ActiveModel::V08::CollectionSerializer"
I was like: "What the F is that?" I guess we should having not such non-sense migration.

We are earth people. We want to get our code align with new 0.10.x structure (so at least we will be able to do again when there is 0.20.x)

Another guide please?

@bf4
Copy link
Member

bf4 commented Mar 21, 2017

Are you offering to help?

@phuongnd08
Copy link
Author

No. I'm asking for help actually. I have no idea of what has been going on with 0.10.0 to offer any help. All I know that migration guide is horrible.

@bf4
Copy link
Member

bf4 commented Mar 21, 2017 via email

@phuongnd08
Copy link
Author

@bkoltai Do you think this migration guide can be rewrite so that no more custom adapter need to be introduced?

@bkoltai
Copy link
Contributor

bkoltai commented Mar 22, 2017

@phuongnd08 that would be great, thanks for contributing!

@phuongnd08
Copy link
Author

@bkoltai okay, so let me begin asking for the new way to do in 0.10.x instead of the old way in 0.8.x: In 0.8.x we return an array of hash [{}, {}] expect it to be rendered "[{}, {}]"
What should we do in 0.10.x instead?

@phuongnd08
Copy link
Author

phuongnd08 commented Mar 22, 2017

Alright, I attempted to do this by introduce this class. I wonder if it should be added to AMS core

class HashSerializer < ActiveModel::Serializer
  def serializable_hash(adapter_options, options, adapter_instance)
    object
  end
end

@phuongnd08
Copy link
Author

Another case is that in 0.8.x we get "null" when serialize nil object. In 0.10.x, what is the new way then? What should we do when an action can either return a model or a nil model (if it doesn't exist?)

@phuongnd08
Copy link
Author

phuongnd08 commented Mar 23, 2017

The old 0.8.x allow nested associations. Now the new 0.10.x doesn't seem to like that. include doesn't work with json and attributes adapter (not sure what hell wrong but I last try with json_api adapter within grape-active_model_serializers it doesn't work neither).

What is the new way in 0.10.x supposed to be? @bf4 @bkoltai
I have to make a hack that instead of declare has_one :my_association in the serializer, I declare attribute :my_association and call this:

def my_association
  MySerializer.new(object.my_association, scope: scope).as_json
end

@bf4
Copy link
Member

bf4 commented Mar 23, 2017

@phuongnd08 In the Rails JSON renderer ( https://github.com/rails/rails/blob/4-2-stable/actionpack/lib/action_controller/metal/renderers.rb#L113-L114 ) , which AMS uses, strings are ignored. So, you can always nil.to_json or [{},{}].to_json or [{name: :benjamin}].to_json]

I'm sure you recognize that using AMS to render primitives like hashes and arrays isn't a good use of a library designed to serialize objects.

@bf4
Copy link
Member

bf4 commented Mar 23, 2017

@phuongnd08 I haven't compare the different AMS versions, but if you're saying that 0.8 renders primitives, then saying that 0.10 does not might be a useful PR. Or anything that you would have liked to see here.

I'm guessing the current wording was insufficient:

No default serializer when serializer doesn't exist

Closing since I think the issue is resolved, but please continue to discuss or reference this.

For posterity, I"m going to link to the current ref of the guide https://github.com/rails-api/active_model_serializers/blob/80e470dcdb8b724990baf3c90d7300884c4395bf/docs/howto/upgrade_from_0_8_to_0_10.md

@bf4 bf4 closed this as completed Mar 23, 2017
@phuongnd08
Copy link
Author

I am not into keeping the 0.8.x way. We want to move forward to 0.10.x. Just need to know how. I think there is still issues to be discussed like the nested associations. Please account that many (majority?) of the user of AMS is just like me, they want to learn the way the library is supposed to work and how to hook into their project. Writing a custom adapter just to get thing backward compatible doesn't sound like a migration at all, it like hacking against your own code when you can just change it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants