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

upgrade doc #1290

Closed
wants to merge 6 commits into from
Closed

upgrade doc #1290

wants to merge 6 commits into from

Conversation

shicholas
Copy link
Contributor

I recently upgraded my .8 ams app to the latest and found the following helpful. Hope someone can find this useful too.

I also feel I'm missing something when it comes to creating an ArraySerializer Adapter and would appreciate some help.

@beauby
Copy link
Contributor

beauby commented Oct 22, 2015

Thanks @shicholas, that's really helpful! It might even make sense to provide a legacy json_08 adapter in 0.10 to make the migration even easier. I know this is something that has been talked about, but I don't think anyone has taken a crack at it as of yet.
We could discuss about the ArraySerializer on our Slack.

@shicholas
Copy link
Contributor Author

do I need to an invite to join slack?

@joaomdmoura
Copy link
Member

@shicholas: https://amserializers.herokuapp.com/ (auto invite)

###### Base Adapter

A base adapter takes a ``ActiveModel::Serializer`` instance, and creates a hash
used for serialization in its ``serializeable_hash`` method. A base adapter
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo serializable

@bf4
Copy link
Member

bf4 commented Oct 22, 2015

This would be fantastic as code with a PR and tests. If we can version APIs, why not adapters?

@shicholas
Copy link
Contributor Author

I don't know about version .9, but maybe for .8 we could have these adapters:

  • Version8::BaseAdapter (with a root element)
  • Version8::BaseAdapterWithoutRoot
  • Version8::CollectionAdapter
  • Version8::CollectionAdapterWithoutRoot

I feel most apps I've worked with use version .8 with the root adapter, and since root shouldn't be defined per serializer anymore, I chose these four adapters.

EDIT: After talking to @beauby, I realize that I shouldn't distinguish between Collection and Base adapters b/c serializable_hash has a check if the serializer responds_to :each?. I updated the doc consequently.

could look like this:

```ruby
class MyApp::BaseAdapter < ActiveModel::Serializer::Adapter::Base
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: ;:. why not try adding this code with tests?

I'm thinking

# lib/active_model_serializers/v08/serializer.rb
# lib/active_model_serializers/v08/collection_serializer.rb
# lib/active_model_serializers/v08/adapter.rb
module ActiveModelSerializers::V08
  # Serializer might not need anything, but good to have defined
  Serializer = ActiveModel::Serializer
  CollectionSerializer = ActiveModel::Serializer::CollectionSerializer # well, it's Array now, but hopefully we'll get that renaming pr soon
  class Adapter < ActiveModel::Serializer::Adapter::Base
    # whatever methods you need
  end
end

And test like the other serializers and adapters

adapter = :'v08/adapter'
serializer = :'v08/array_serializer'
each_serializer = :'v08/serializer'
collection_resource = Post.all
expected_response = [{ something }]
assert ActiveModel::SerializableResource.new(collection_resource, serializer: serializer, each_serializer: each_serializer, adapter: adapter).as_json, expected_response

# and

adapter = :'v08/adapter'
serializer = :'v08/serializer'
resource = Post.first
expected_response = { something }
assert ActiveModel::SerializableResource.new(collection_resource, serializer: serializer, adapter: adapter).as_json, expected_response

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though after clicking comment, I realized the options definitions are wrong, should be:

adapter = ActiveModelSerializers::V08::Adapter
serializer = ActiveModelSerializers::V08::ArraySerializer
each_serializer = ActiveModelSerializers::V08::Serializer

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rails-api/ams thoughts on naming, folder etc?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

anything specific? looks ok to me :-\ (except for the crazy long asserts)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the API for the serializer itself changes (namely the root option). How should I address that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bf4:

typo: ;:. why not try adding this code with tests?

sorry I must be blind, I don't see the typo.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code blind
Semicolon in place of colon

Way i wrote was ambiguous

B mobile phone

On Oct 22, 2015, at 12:02 PM, Nick notifications@github.com wrote:

In docs/howto/UPGRADING.md:

+# How to upgrade from previous versions
+
+### Creating a Custom Base Adapter
+
+You can upgrade to ams .10 by creating a custom Base Adapter to
+emulate the response that your application currently serves.
+
+###### Base Adapter
+
+A base adapter takes a ActiveModel::Serializer or
+ActiveModel::Serializer::ArraySerializer instance, and creates a hash
+used for serialization in its serializable_hash method. A base adapter
+could look like this:
+
+```ruby
+class MyApp::BaseAdapter < ActiveModel::Serializer::Adapter::Base
@bf4:

typo: ;:. why not try adding this code with tests?

sorry I must be blind, I don't see the typo.


Reply to this email directly or view it on GitHub.

@shicholas
Copy link
Contributor Author

closing, I think a V8 adapter is a good idea (though still would like to know how to deal with the issue of the root option) and I feel the docs that will result from #1317 will trump this PR.

Also, I feel with the code somewhat in flux, e.g. the namespacing changes I have been reading, maybe the v8 adapter should be written after the api stabilizes?

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.

5 participants