-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Support Rack::Sendfile middleware #1280
Conversation
Interesting. Lets start with these:
|
Hi! Sorry, for long response. I wrote test |
Yes, please and thank you @lfidnl. I am glad this gets cleaned up and tested. |
@dblock, I've added test, update CHANGELOG and fix build. I hope, I done everything well :) |
27b25aa
to
f450eea
Compare
@@ -2328,6 +2328,21 @@ class API < Grape::API | |||
end | |||
``` | |||
|
|||
For using Rack::Sendfile middleware, specify `to_path` method in your |
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.
But this is not true, no? The code change makes it that I don't have to worry about file streamers or any other thing, I can just plop Rack::Sendfile
in there and it will "just work", right? Maybe just say that this is compatible with Rack::Sendfile and add a note on what that does?
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.
@dblock , thanks for comment! I mean that agrument of file
dsl method should respond to to_path
method (of couse use Rack::Sendfile
shoule be applied in API class).
So it can be FileSteamer
with realized to_path
method or other object with to_path
method. I uses "file streamer" notion for supporting article in the same style.
I think that adding information about Rack::Sendfile
will be overkill. I can replace information with this:
For using Rack::Sendfile
middleware, specify to_path
method in your file streamer class which returns path of served file:
class FileStreamer
# ...
def to_path
@file_path
end
# ...
end
Note: don't forget turn on Rack::Sendfile
middleware in your API:
class API < Grape::API
use Rack::Sendfile
end
If you want to just add Rack::Sendfile
and I starts to work see my comment on below :)
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.
Ok I get it. I think it should look like the text above, which is If you want a file-like object to be streamed using Rack::Chunked, use stream.
Maybe
If you want to take advantage of Rack::Sendfile, which intercepts responses whose body is being served from a file and replaces it with a server specific X-Sendfile header, ...
Update it as you see fit and I'll merge.
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.
Ok. I'll update readme during for couple days
Tests are good. See my above note on README, squash commits please. |
I think we must rewrite Grape's api for serving file like in Rails framework: get '/file' do
file '/path/to/file'
end
Lets create new issue or I'll try to provide propose with Pull Request after closing this PL :) |
👍 on extending |
f450eea
to
0c827c1
Compare
README updated |
Support Rack::Sendfile middleware
Merged, thanks. |
Hello.
I've added support of
Rack::Sendfile
middleware. Before this fixRack::Sendfile
does not apply strategy of adding X-Accel-Redirect (or X-Sendfile) header, because Grape returns simpleRack::Response
which doesn't respond toto_path
method. So I've implemented simpleSendfileReponse
class which responds toto_path
.