Skip to content
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

Set Expires header when caching responses #2182

Merged
merged 4 commits into from
Dec 30, 2021
Merged

Set Expires header when caching responses #2182

merged 4 commits into from
Dec 30, 2021

Conversation

RealOrangeOne
Copy link
Contributor

Browsers are rather smart, but also dumb. This uses the Expires header
alongside cache-control to better prompt the browser to actually
cache.

Unfortunately, firefox still tries to "race" its own cache, in an
attempt to respond to requests faster, so still ends up making a bunch
of requests which could have been cached. Doesn't appear there's any way
around this.

See also #1453 (comment)

Browsers are rather smart, but also dumb. This uses the `Expires` header
alongside `cache-control` to better prompt the browser to actually
cache.

Unfortunately, firefox still tries to "race" its own cache, in an
attempt to respond to requests faster, so still ends up making a bunch
of requests which could have been cached. Doesn't appear there's any way
around this.
src/util.rs Outdated Show resolved Hide resolved
Co-authored-by: Daniel García <dani-garcia@users.noreply.github.com>
@BlackDex
Copy link
Collaborator

Looks good indeed. But do we really need an extra crate for this??
Can't this also be done with Chrono maybe?

@RealOrangeOne
Copy link
Contributor Author

do we really need an extra crate for this

It's already depended on by hyper to do the same thing, but I couldn't see an interface to get to the implementation we need which is exposed through rocket. I agree it's not ideal. I'll take another look and see what I can find.

@RealOrangeOne
Copy link
Contributor Author

@BlackDex I've written my own implementation, which seems to work as expected. I'm not entirely convinced it's better, as httpdate is already a dependency of hyper, do it doesn't bloat the binary. Unfortunately hyper doesn't expose any of httpdate's functionality, so the only option is to either depend on it ourselves, or write a custom implementation.

Copy link
Contributor

@williamdes williamdes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice to not have one more dependency

@dani-garcia
Copy link
Owner

Thanks, we can keep it this way, if it causes issues we can consider adding that dependency back.

@dani-garcia dani-garcia merged commit 9203719 into dani-garcia:main Dec 30, 2021
@RealOrangeOne RealOrangeOne deleted the cache-expires-header branch December 30, 2021 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants