search

loco-rs / loco

🚂 🦀 The one-person framework for Rust for side-projects and startups
https://loco.rs
Apache License 2.0
5.45k stars 235 forks source link

Change static asset fallback to not return a 404 #991

Closed Innominus closed 1 day ago

Innominus commented 5 days ago

~~Right now, if you're using the static assets middleware to fallback to an index.html file for an SPA configuration, it will correctly return the index.html file, but with a 404 status code. The middleware should be configurable to allow you to return a 200 OK instead as that is the correct semantics. Happy to change this PR however maintainers would like, and change the setting name 'should404' - there's probably something better.~~ image ~~This would be a breaking change to current fallback configurations_~~ I have updated the PR to just return the unaltered status code, rather than a 404.

isaacdonaldson commented 5 days ago

I think instead of adding a boolean to the existing fallback, it's a better idea to have the fallback use a 200 status, and add another option not_found to handle the 404 case more explicitly. It also mirrors what tower does a bit closer as well.

So you'd have fallback: "index.html" that would help with SPAs and have status code 200, and have not_found: "404.html" that would deal with 404s explicitly.

Maybe @jondot or @kaplanelad have a different opinion though.

Innominus commented 5 days ago

I think instead of adding a boolean to the existing fallback, it's a better idea to have the fallback use a 200 status, and add another option not_found to handle the 404 case more explicitly. It also mirrors what tower does a bit closer as well.

So you'd have fallback: "index.html" that would help with SPAs and have status code 200, and have not_found: "404.html" that would deal with 404s explicitly.

Maybe @jondot or @kaplanelad have a different opinion though.

Yeah, I think you're right. I might just swap it out to just be .fallback() instead of the not_found_service(). To me, having a fallback return a file with a 404 status code doesn't make much sense to me. There is already a fallback middleware that 404's and returns a html page of your choosing, I think maybe this should just return a 200 with no configuration.

jondot commented 1 day ago

Thanks for the work and discussion. Your reasoning seems correct!