-
Notifications
You must be signed in to change notification settings - Fork 526
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
Rails 5 compatibility #752
Conversation
I’m not sure exactly what needs to change. Here’s what another gem did to fix it: jfirebaugh/konacha#216
- require Rails 5 since Rubygems doesn’t allow us to only require activemodel-serializers-xml when Rails is version 5 - require Ruby 2.2 since that’s what Rails 5 requires
I just guessed at this — it was getting a no method error without this change
since Rails 5 proper has been released, and mintiest-rails 3 has been as well.
ActiveModel::ArraySerializer -> ActiveModel::Serializer::CollectionSerializer The only thing I’m not sure about is making AMS a runtime dependency.
Thanks @seanlinsley! |
Okay. I'm currently trying to pin down where exactly Draper interfaces with AMS, as well as activemodel-serializers-xml, to determine if we can just list them in the README or if they actually need to be runtime dependencies. |
@shaneog I reverted that change, so now AMS isn't required at runtime. The only question is whether the initializer is necessary for 0.9.x. Could you try this branch out on your app? |
end | ||
|
||
if defined?(ActionMailer::TestCase) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May I ask why you are making this change? It breaks testing apps without ActionMailer included, right?
NameError: uninitialized constant ActionMailer
<hidden>/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/bundler/gems/draper-de70008a5e68/lib/draper/test_case.rb:36:in `<top (required)>'
<hidden>/.rbenv/versions/2.3.1/lib/ruby/gems/2.3.0/bundler/gems/draper-de70008a5e68/lib/draper/railtie.rb:21:in `block in <class:Railtie>'
etc..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My mistake, I didn't realize that this file was something that other people's test suites relied upon. I'll undo this change.
@seanlinsley Apologies for the delay - this looks like it works ok for me, although I'm still waiting on another gem before I can do in-depth testing. |
@seanlinsley trying to use draper's rails-5 branch with our app and we get this when trying to run
Note that our ApplicationController extends from ActionController::API instead of ActionController::Base and API does not appear to have view_context available. If I switch to extending ActionController::Base I can load the console but of course that's not great since rails 5 brings rails-api in directly for API-only use cases. I thought I saw some other older PRs that tried to address this view_context problem when using the old rails-api gem w/rails 4x but for whatever reason it does not work with rails 5. |
@bdmac that doesn't seem related to my changes, and I'm not sure how to fix it without more of the stack trace. Could you open a ticket for that issue with more details? |
Sure can although repro should be pretty easy, just switch your application controller to inherit from API instead of Base and try to open rails console. Will get stack trace in a bit. |
so they’re not fuzzy on retina displays
If you are going to drop rails 4.x, you may want to change the major version, so people still running 4.x can easily peg their gemfile to a draper version |
@kbrock yep, the plan is to bump to 3.0.0 |
I just got access to Rubygems so I'm going to merge this and publish a prerelease version. |
3.0.0.pre1 has been released |
yay! |
I'll check out the prerelease this week and report back if I see any issues. Thanks! |
Note that for simplicity's sake, this PR drops support for Ruby < 2 and Rails < 5.
It also makes Active Model Serializers a runtime dependency. I'll try and undo that if I can.