Introduce http feature for http crate types
#477
Conversation
|
@spigaz this is passing all tests except for redirects and websockets. I also need to see how we handled the |
|
Tests are now all passing, but I'd like to attach the |
|
@kflansburg That's awesome! You can, but you can you can also added it as a PR latter on. |
|
Ok, I think this is in pretty good shape, and touches the least number of files. I'm probably gonna knock off for today, but I think all that is left is docs. |
|
@kflansburg Great! Thanks! It seems at least one person is trying to use the other attempt based on http 0.2, I don't think documentation is a concern for now, besides its hidden behind a feature flag. I'm going to try to use this on my app and see what happens. |
|
I've added an |
@kflansburg Sorry, I missed this comment, I was busy with the example from @avsaase but I'm guessing you already fixed most of the issues his example had, so no point in that. When I got to that problem, my original idea was to accept the the trait in the response. I have checked and it seems common to define the body as T and the response as Response<T> with the some kind of restriction, in this case something like T: http_body::Body, instead of forcing the conversion to worker::Body. When you have to convert to ::worker::worker_sys::web_sys::Response you will have to use a method of the trait anyway to perform the conversion, I'm not sure you can't convert directly at least in some cases, instead of always converting to worker::Body. |
There was a problem hiding this comment.
The changes here look good. I think it makes sense moving forward with a subset of the original changes so we can get the ball rolling and then implementing more features in separate PRs.
What's the plan in regards to the http feature? Do we keep it around for long or only until the wanted changes are implemented?
My hope would be to remove the feature in the next few months once the remaining |
|
@kflansburg I already ported by app, only had to add: I can open a PR, but I'm not sure it won't cause a distraction. Aside from that, it was a clean replacement, no issues, it simply worked! Thanks! |
|
Ok, I think this is in a pretty good spot. I will leave it for a day and see if I can get another contributor to approve. |
kflansburg
changed the title
http attempthttp feature for http crate types
|
I'm also porting a worker to this PR and I'm running into an issue that |
|
We probably need to add the unsafe Sync / Send impl to more places, but I think we should land this soon and deal with those as they come up. |
| /// A struct for invoking fetch events to other Workers. | ||
| pub struct Fetcher(worker_sys::Fetcher); | ||
|
|
||
| #[cfg(not(feature = "http"))] |
There was a problem hiding this comment.
We might want to do the same thing in global.rs with Fetch, on the other hand I really don't like the design of Fetch and we can save it for when we can think of a better API that's more idiomatic before we commit to anything.
There was a problem hiding this comment.
Ah, good catch, did not realize there are two implementations.
There was a problem hiding this comment.
I started going down this route and it turned out to be a can of worms, so I think this will need to be done in a follow up PR.
Mainly, it would involve a lot of changes in the many places that the global fetcher is used.
It also forced me to think about being able to convert between old and new types by reference. Supporting that is pretty painful and I'm on the fence whether it is even reasonable. So we may want to change Fetch::fetch_with_request to take an owned request if that seems ok. The only use case I can think of having a reference here is making multiple fetches with the same request, which must also not have a body. I'm not sure how common that is.
|
@kflansburg Thank you! I know a lot is ahead, but this both a small step and a giant leap! Congratulations! |
* http 1.1 * http feature flag * Fetcher http flag * Implement todos * Implement redirect / ws / abort signal * Add http tests to CI * Add Cf context to http::Request * Fix test working directories * Axum example * fix clippy in CI * Handle generic http_body return type * Documentation * Remove unwraps * Tweak introduction version in docs * Final touches
This PR will introduce a new crate feature,
httpwhich will allow users to leverage widely used types from thehttpcrate, instead of the custom types implemented inworker.This makes it very easy to use common Rust frameworks like
axumto implement routing, etc.Functions are provided to convert between new and old types so that end-user code can be converted incrementally.
The plan is to slowly convert more of the
workercrate to usehttpover the coming months in a series of non-breaking PRs, and eventually remove proprietary HTTP types when the conversion is complete in a single minor / breaking version bump. This should give consumers of this crate time to convert their applications such that the only real change is removing the feature flag and making types from thehttpcrate the default.