jistr
Fixes issue #1
This fixes issue #1! Mobvious::Manager now passes Rack::Response objects correctly.
Now fixed! I'd made the incorrect assumption that ActionDispatch::Response inherited from Rack::Response
Thanks for getting into this.
Assigning body to response variable looks funny, but works because ActionDispatch::Response returns self in this place. I'll probably look into this a bit more and see if I can write it a bit more clear/bulletproof. I almost dismissed this as a wrong solution, because I would have never imagined that ActionDispatch::Response#to_ary returns [@status, @header, self]. Took me some time to find out :)
According to https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/http/response.rb, ActionDispatch::Response seems to duck type to Rack::Response and whatever is acceptable as body. In any case, it gets passed as body into
My solution assumes that anything which is not a plain Array must duck type to ActionDispatch::Response, which is technically wrong, although if you pass something that is not a plain ruby Array into the Rack::Response constructor, it's going to come out with class Array.
I think Rack and/or Rails might have failed us here.
Hey John, I hope this should do: https://github.com/jistr/mobvious/commit/e225771e4d1118484928726d4db66be56b8229ae
Could you please verify that it works? You can put this into your Gemfile to test it:
gem 'mobvious', :git => 'git://github.com/jistr/mobvious.git', :branch => 'actiondispatch'
Hi @jistr, thanks for trying to fix all the issues! Unfortunately, the fix you've made doesn't work for me. I don't think you should be creating a new Rack::Response at all, because you're going to lose any information stored as instance variables on an object that is expected to duck type. Also, imagine the performance hit if all rack middleware rewrote every response. You might also break streaming. Instead, I think you should be writing to it. I added a spec here in my original pull request:
https://github.com/johncant/mobvious/blob/1551f8a413aebc54b1eabde8c3167fcf4f5f2547/spec/mobvious/manager_spec.rb
which checks that Rack::Response objects are preserved. However, this isn't enough, since ActionDispatch::Response does not inherit from Rack::Response.
Hmm :( I'm not creating a new Rack::Response if the thing returned from app already behaves in a way compatible with Rack::Response, which should be the case of ActionDispatch::Response. This part takes care of it: https://github.com/jistr/mobvious/blob/actiondispatch/lib/mobvious/manager.rb#L27-L31 I'm looking if it responds to #header, #status and #body methods, which ActionDispatch::Response does, and if so, I use it directly instead of creating a Rack::Response. So I wonder what's the practical difference between your solution and mine...
The thing with the spec you added is that it passed even on original Mobvious code, so it doesn't really test the issue in question (unless I messed up somewhere).
And yes, I'm not rewriting response. I return the original thing that the app returns. Also, creating new Rack::Response doesn't rewrite the response, unless I'm mistaken. It's just a wrapper object for more convenient access to the underlying data, but it doesn't create a copy.
You're right. I can't see how your rack_response? method wouldn't detect ActionDispatch::Response. However Rack::Response.new does by definition create a new object of type Rack::Response. It then writes the strings one by one (ok, no actual copying).
My test is wrong because I've been an idiot: https://github.com/johncant/mobvious/blob/master/spec/mobvious/manager_spec.rb#L16 That mock should return @rails_return_value, not @return value.
I'll take another look at this stuff later.