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

Use http-1.0.0 for http-cache-tower #70

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

SebRollen
Copy link

@SebRollen SebRollen commented Jan 26, 2024

WIP branch for updating the refactor branch to use http-1.0.0, per our discussion in #4.

If accepting the Clone bound on ReqBody is acceptable, it simplifies the passing of the request to the future quite a lot.

If this looks promising to you, I can also try to tackle updating the future into a state-machine so that we can poll other futures inside of it. I know the refactor branch is quite far behind the main branch though, so if it makes more sense to work off of that one, let me know. I might need some pointers in terms of which methods need to be called inside the future since I'm not too familiar with http-cache internals.

Adds cache mode fixes, overridden_cache_mode method, and impl Default for CacheMode
@SebRollen SebRollen changed the base branch from refactor to main January 27, 2024 02:23
@06chaynes
Copy link
Owner

06chaynes commented Jan 27, 2024

Pushed a new branch that I think might ease this implementation a bit, at least I think it might in regards to running the async code blocks https://github.com/06chaynes/http-cache/tree/remove-async-trait

@06chaynes
Copy link
Owner

One prerequisite to merging this will be the Reqwest update to use hyper v1.0.0, which thankfully seems to be underway here https://github.com/seanmonstar/reqwest/tree/hyper-v1

After updating in my local branch everything but the http-cache-reqwest crate seems to be good to go, so once Reqwest itself is updated it should be fairly straightforward to complete on this end.

match this.state.as_mut().project() {
ResponseFutureProj::Init => {
let request_parts = this.req.clone().into_parts().0;
let fut = this.cache.before_request(&request_parts);
Copy link
Author

Choose a reason for hiding this comment

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

@06chaynes, I took this line from the commented-out code in your branch, but there's no before_request method on cache. Assuming it's because the previous branch was out of date, what should be called here instead?

If you have an async version of the code, I can probably translate it to the hand-rolled future

Copy link
Owner

Choose a reason for hiding this comment

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

@SebRollen yeah in that branch I was experimenting with a different implementation that I decided (at least for now) not to go with, though I did bring over a handful of changes.

Here is the implementation for Reqwest that has the logic as it currently is https://github.com/06chaynes/http-cache/blob/develop/http-cache-reqwest/src/lib.rs#L212

First the Middleware trait implementation is constructed, then the request is checked to determine if it's cacheable. If it isnt cacheable we just run the run_no_cache method of the HttpCache struct, handle the response normally, update the two cache headers, and pass it through. If it is cacheable we run the run method of the HttpCache struct and pass through the result.

Copy link
Owner

Choose a reason for hiding this comment

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

pushed a commit to update the core and other crates http and http-cache-semantics versions

@06chaynes 06chaynes changed the base branch from main to develop January 29, 2024 01:55
@06chaynes
Copy link
Owner

Sweet, reqwest has been updated. Just need a new reqwest-middleware release and this particular change should be about ready

@NOBLES5E
Copy link

NOBLES5E commented Jul 2, 2024

@06chaynes It seems that reqwest-middleware has been released

@06chaynes
Copy link
Owner

@06chaynes It seems that reqwest-middleware has been released

That comment was in regards to updating http, which has already been merged in

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.

3 participants