Skip to content

Remove a hard dependency on ActiveModelSerializers #136

Closed
wants to merge 3 commits into from

4 participants

@rykov
rykov commented Feb 16, 2013

Based on my own experience and a few of blog posts, it looks like people are using various solutions (Rails-API, etc) to build Ember-compatible APIs in Rails, not just ActiveModelSerializers. Nothing in ember-rails requires to have the AMS dependency, so I'd like to raise for consideration removing this hard dependency.

It's a great convention when starting a new app, but I think many developers will try to add ember-rails to an existing Rails application, and if their experience is anything like mine, it can mess up existing API implementations. In my example, I had my own responder that was overridden by AMS. I found no way to turn it off.

To solve this problem, I forked the gem without the AMS dependency as ember-rails-lite, but figured I'll submit this for general consideration.

@stefanpenner
Ember.js member

I am pretty sure we want to keep AMS as part of this gem. I think the correct approach is for either your custom responders to play nicely with AMS, or AMS to play nicely with custom responders.

@rykov
rykov commented Feb 27, 2013

Convention without configuration?

@stefanpenner
Ember.js member

@rykov I believe my previous comment remains.

@rykov
rykov commented Feb 27, 2013

I'm referring to my original statement that automatically modifying default ActionController serialization behavior, especially in existing applications, without any way to disable it, is both dangerous and not very principle-of-least-surprise-like. It's very possible to break existing APIs when trying to add ember-rails to an existing Rails app, which I presume is a legitimate use-case of this gem.

I understand your comment that AMS is here to stay, but it should either be inserted/activated via the ember:bootstrap generator or have a configurable option that gates the require call.

@stefanpenner
Ember.js member

@rykov I agree with you totally, the above commit though simply removed AMS while not adding it to the generator. If you can provide a solution that doesn't disrupt the typical happy path, but offers an easy opt-out of AMS, I will be very supportive.

---- out of scope for this issue, but something we should disucss ---

That being said, I would also like AMS to play nicely with other custom responders. It is entirely possible, although not ideal, to have part of your app (a new or newly refactored section) using AMS, and an older part not. I feel strongly that AMS should be cooperative citizen. We can likely get the AMS maintainers support on this.

@carlosantoniodasilva

Hey guys, just adding my 2 cents: I think it's fine for ember-rails to choose AMS and ship with it as a dependency, but it should be definitely possible to use AMS partially in your application, and not touch other parts where you don't want it.

If AMS is entering your way somehow, I think it's something it'll have to be handled from its side, allowing you to somehow disable this automatic serialization behavior that @rykov talks about, and only using it where the user actually wants it.

Might be interesting to hear thoughts from AMS side by opening up an issue there. Cheers!

@rykov
rykov commented Mar 1, 2013

Despite all the controversy behind the default choices for Rails, I really like the fact that many of the defaults I can disable by deleting lines from the Gemfile. This goes for both jbuilder and turbolinks (I believe). Out of the options I offered, I think making AMS activation a part of ember:bootstrap is my favorite. I just haven't had the time to code it up yet.

@carlosantoniodasilva

Yup, seems like a good plan :)

@stefanpenner
Ember.js member

seems like we are all in agreement.

  1. AMS gem should likely be part of the generator,
  2. we need a good way to message 1. not interfere to much with existing ember-rails users
  3. We should figure out why AMS and your custom renderer do not play nicely, and open issues accordingly.

@rykov i'll gladly work with you to ge this all sorted out, and @carlosantoniodasilva as usual thank you for your input.

@rykov
rykov commented Mar 2, 2013

Here's my first stab at it. rykov/ember-rails-lite@028c173 Thoughts?

@stefanpenner
Ember.js member

@rykov seems good. This wil break many peoples current installs when they upgrade, so we need a good way to message/inform/document this change.

@rykov
rykov commented Mar 2, 2013

@stefanpenner I can put this in a post_install_message in the .gemspec and we can pull it out later.

@stefanpenner
Ember.js member

@tchak thoughts?

@stefanpenner stefanpenner reopened this Mar 3, 2013
@tchak
Ember.js member
tchak commented Mar 3, 2013

I would think that a better fix is like suggested @carlosantoniodasilva, to fix AMS so it would not break custom serializations in existing apps. But in the mean time, I have no problem with this change. So if @stefanpenner is ok with it I am too.

@rykov could you check travis failures ?

@stefanpenner
Ember.js member

@rykov mind sharing ur custom renderer?

@rykov
rykov commented Mar 3, 2013

@tchak I agree -- I think AMS should be able to be seamlessly dropped-in, but both solutions are not mutually exclusive. Being a part of the generator will prevent forcing AMS without ability to turn it off. This follows the Rails and Ember ethos (as I mentioned), with examples being jbuilder, turbolinks, and RESTAdapter, which are all defaults but replaceable.

I've fixed the tests.

@rykov
rykov commented Mar 3, 2013

@stefanpenner @tchak I think the fundamental mistake in AMS implementation is monkey-patching the low-level render :json => ... handler. This method is used in all sorts of contexts (in Rails and existing applications) and should not be modified.

The correct way to implement a custom serialization strategy is to extend ActionController::Responder, set ApiController.responder = AMS::Responder, and encourage users to call respond_with(@posts) instead of render :json => @posts. This is the reason Responders were made. This implementation will allow per-controller choice of responder (if developer wants to pick and choose) and prevent breakage of existing apps.

More info: http://api.rubyonrails.org/classes/ActionController/Responder.html

This is actually what's breaking for me -- I built a super simple custom responder. And when I call respond_with(@posts), the underlying implementation calls render :json => ... with the results of my responder. The result, however, is subsequently modified by AMS -- which is what's causing the problem.

Here is the responder: https://gist.github.com/rykov/fa547aac8b78f5eb200e

@rykov rykov referenced this pull request in rails-api/active_model_serializers Mar 3, 2013
Closed

AMS should be a Responder, rather than modify render :json behavior #217

@rykov
rykov commented Mar 3, 2013

As I mentioned a couple replies ago, we have two discussions happening here:

  1. Ember-Rails's hard dependency on AMS
  2. AMS playing nice with other serialization strategies

I'd like to independently wrap up 1 here, and we can continue 2 at a newly created rails-api/active_model_serializers#217

@tchak
Ember.js member
tchak commented Mar 4, 2013

I completely agree with @rykov on the fact that AMS should not monkey patch and implement a proper responder.

If @stefanpenner is ok I will merge this PR.

@rykov
rykov commented Mar 13, 2013

@tchak @stefanpenner This is getting moldy -- anything else I need to do to get this merged?

@rykov
rykov commented Mar 14, 2013

Just wanted to throw in an update that AMS just merged my changes for a proper Responder and ability to disable the monkey patch - rails-api/active_model_serializers#228

@stefanpenner
Ember.js member

@rykov 🍻 ... I will take a thorough look after work today.

@stefanpenner stefanpenner was assigned Mar 14, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.