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

Add notes about what's going on with AMS #2169

Closed

Conversation

NullVoxPopuli
Copy link
Contributor

@NullVoxPopuli NullVoxPopuli commented Aug 1, 2017

rendered

personally, I have opinions, and one of those is that anyone using a json api should use JSONAPI.org style json.

I don't think it makes sense for for AMS to continue with the adapter flexability either -- unless someone had some very specific scenario where they needed multiple adapters in the same project.

Update:

  • if AMS is to continue, it needs to be A LOT simpler. Current plan is to do the heavy lifting with jsonapi-rb and transform based on chosen adapter.

TODO:

  • Create Orphan Branch including the benchmark.
  • Discuss next Iteration of AMS

@mention-bot
Copy link

@NullVoxPopuli, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bf4, @steveklabnik and @ggordon to be potential reviewers.

README.md Outdated
Every scenario builds and renders [JSONAPI.org](jsonapi.org) documents of 301 records.

eager means eager loaded data (no db hits).
The benchmark for this can be found [here](https://github.com/NullVoxPopuli/rails-NPlusOneTests/blob/master/serialization_benchmark.rb)
Copy link
Member

Choose a reason for hiding this comment

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

should ref a specific commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks

README.md Outdated
@@ -1,4 +1,56 @@
# ActiveModelSerializers
# ActiveModelSerializers (Deprecated)
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to declare AMS dead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the description for some clarity in my intent.

like, it sucks, cause it has high visibility, but @beauby did a GREAT job with jsonapi-rb.

I just don't feel like another re-write is going to give people confidence.
migrating to jsonapi-rb is pretty straight forward, too.

tonight, I'm working on some compatibility stuff to allow smooth migration from AMS to jsonapi-rb.

Copy link
Member

Choose a reason for hiding this comment

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

Instructions on migration would be fantastic

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's also more than serialization.

but yeah, I'll submit a separate PR for transition instructions to both AMS and jsonapi-rb when I get that stuff working. :-)

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

Copy link
Member

@bf4 bf4 left a comment

Choose a reason for hiding this comment

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

Good start of suggesting alternatives, though I disagree that we should call out jsonapi-rb as the sole alternative, even if it is a spiritual sucessor

README.md Outdated
# ActiveModelSerializers
# ActiveModelSerializers (Deprecated)

Please use [jsonapi-rb](https://github.com/jsonapi-rb) ([docs](http://jsonapi.org))
Copy link
Member

Choose a reason for hiding this comment

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

This isn't a totally fair recommendation since not everyone wants JSON:API, and jsonapi-rb isn't the most-obvious choice for JSON:API. If we want to be fair, we should suggest ROAR, jsonapi-resources, etc.

I'd be happy to add an alternatives section down where we mention dev might not be released, but generally I think this recommendation is too specific for one library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ROAR and jsonapi-resources provide a layer on top of rails, whereas jsonapi-rb and ams are just (de)serialization

README.md Outdated
Every scenario builds and renders [JSONAPI.org](jsonapi.org) documents of 301 records.

eager means eager loaded data (no db hits).
The benchmark for this can be found [here](https://github.com/NullVoxPopuli/rails-NPlusOneTests/blob/2a5ebfec262e53d8bcb7f3308388fc5ba64f599d/serialization_benchmark.rb)
Copy link
Member

Choose a reason for hiding this comment

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

maybe this could be an (orphan) branch on AMS since it's a recommendation from AMS -> something else?

@NullVoxPopuli
Copy link
Contributor Author

@bf4 I guess, I'm back to struggling with what the goal of AMS is?

@beauby
Copy link
Contributor

beauby commented Aug 2, 2017

My 2cts: jsonapi-rb was intended as a jsonapi-only faster/smaller/more flexible alternative to AMS, which I think it succeeded being. That said, quite a lot of people still use AMS for non-jsonapi stuff, so I don't think AMS should be declared dead.
However, the initial idea that one would have a common interface to declare serializers for various formats has been implemented in a much better way in ROAR IMO, so if AMS is to keep going forward, I believe its scope should be narrowed down to simple serialization (i.e. attributes/json adapters).

Regarding suggesting alternatives for JSONAPI, here are those I have worked with:

  • (de)serialization only:
    • jsonapi-rb (or jsonapi-rails for rails bindings)
    • jsonapi-serializers (small and efficient, but has a few bugs, plus a dependency on activesupport)
    • roar-jsonapi (the jsonapi adapter is not production-ready and has many bugs)
  • higher level frameworks (with a higher-level notion of resource, automatic handling of query parameters, etc.)
    • jsonapi_suite (which uses jsonapi-rb under the hood)
    • jsonapi-resources

@NullVoxPopuli
Copy link
Contributor Author

NullVoxPopuli commented Aug 2, 2017

I gave up on the translator for jsonapi-rb <-> AMS. It's easy enough to just convert things / add a second set of serializers gradually. (I also used this as an opportunity to make drawers a little more configurable )

In case anyone else finds this, the only real differences I had to deal with:

  • serializers need a type explicitly set
  • implicit attribute definition overriding via method definitions (i.e. attribute :method_name along with def :method_name; ...; end) is not supported and must be replaced with value overriding in a block (i.e. attribute(:method_name) { ... })
  • It's better to specify explicit serializers as the string form to avoid circular dependency issues

@NullVoxPopuli NullVoxPopuli changed the title Update Readme to notify people of better solution for JSONAPI Add notes about what's going on with AMS Aug 2, 2017
@NullVoxPopuli
Copy link
Contributor Author

@bf4, I created the orphan branch, and updated this PR to reference that :-)

@bf4
Copy link
Member

bf4 commented Sep 19, 2017

### Motivation for yet another re-write
- The maintainers of AMS have cycled in and out a few times, and the code base has become unwieldy
- 0.10.x's architecture could not handle all the feature requests in an elegant way.
- performance of AMS is bad/slow.
Copy link
Member

Choose a reason for hiding this comment

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

These notes are good

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.

None yet

4 participants