-
Notifications
You must be signed in to change notification settings - Fork 166
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 ServeDir::fallback
#243
Conversation
This also allows showing a custom 404 page for a |
Oops |
Yep it does!
Hm what do you mean? Like not responding with |
hehe not quite ready for review yet :P |
Misclicked in mobile UI, not sure how to change back 😅 Edit: Ah you did already. |
I've fixed it :) |
Yeah, since you'd want the custom not-found page to still have the 404 status code even when served from an on-disk file successfully. |
Ahh I see. So like |
Maybe ServeDir::new("...").fallback_with_status(ServeFile::new("not_found.html"), StatusCode::NOT_FOUND) That way we wouldn't need anything new in |
I guess the question there would be when do you apply the custom status code? Probably if the status code from the inner service is anything 2xx? Also, if this is not attached to |
I'll try and find some time to work on this soon but might just make a breaking release next week to get #237 out regardless if this has landed or not. |
Made a PR to add a I don't think a more general fallback mechanism is possible since it requires cloning the request (to |
type Response = Response<ResponseBody>; | ||
type Error = io::Error; | ||
type Future = ResponseFuture; | ||
type Future = ResponseFuture<ReqBody, F>; |
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.
These new type params makes this a breaking change.
This is finally ready for review! |
Co-authored-by: Jonas Platte <jplatte+git@posteo.de>
} | ||
|
||
fn call(&mut self, req: Request<ReqBody>) -> Self::Future { | ||
let mut full_path = match self.variant.full_path(&self.base, req.uri().path()) { | ||
// `ServeDir` doesn't care about the request body but the fallback might. So move out the |
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.
All methods that are reasonable for asset routes don't have a body anyways, right? By the way, does ServeDir
still serve files for non-GET requests? I think it did that at some point.
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.
All methods that are reasonable for asset routes don't have a body anyways, right?
Yes. At least I can't think of a use case for serving a static file and having a request body.
By the way, does ServeDir still serve files for non-GET requests? I think it did that at some point.
It still accepts all methods. Perhaps we should change that to respond with 405 for non-GET
requests.
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.
There is proper handling of HEAD now, right? That should probably stay. Not sure about OPTIONS, probably okay to have it return 405 if the user doesn't have a cors middleware set up.
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.
Apart from latest comments, looks good!
Fixes #240
Example usage:
This will call
ServeFile
if the file is missing.You can also achieve this using combinators from
tower::ServiceExt
but several have asked for this so probably makes sense to add.Note this is different from
axum_extra::routing::SpaRouter
which requires assets to be nested (for example under/assets/*
). This doesn't require that and thus allows serving assets at the top level (i.e.GET /style.css
or smth like that).TODO