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 types directly instead of converting to and from them #13

Open
jyn514 opened this issue Aug 24, 2021 · 6 comments
Open

Use http types directly instead of converting to and from them #13

jyn514 opened this issue Aug 24, 2021 · 6 comments

Comments

@jyn514
Copy link
Contributor

jyn514 commented Aug 24, 2021

We'll still need to convert this in worker itself, because it needs to turn back into a wasm_bindgen::JsValue, but we don't need to add our own Response type, we can just use one from the ecosystem. Same for many of the other types (Request etc).

@nilslice
Copy link
Contributor

This is sort of true - unfortunately the devil is in the details. We do actually need our own types, because the Cloudflare Workers runtime differs in subtle and major ways. The plan is to have our own base types that map 1:1 to the JS types exposed to the runtime, and then have conversion types for each that make sense. We can't just use the http::Request type as our baseline, because it would disallow a user from setting things like RequestInitCfProperties, or inspect request values on our cf object like colo.

I would be glad to know there is a more direct way to support these ecosystem types vs. conversion traits!

One magic way would be to decorate the event macro to use something like:

#[event(fetch, hyper)]
async pub fn main(req: hyper::Request, ...) -> Result<hyper::Response, E> { ... } 

In this approach, the code within the macro would rely on the conversion trait impls on our types to go from one to another. Definitely down-the-line thinking, and will certainly be impacted by end-user feedback.

@jyn514
Copy link
Contributor Author

jyn514 commented Aug 25, 2021

I would be glad to know there is a more direct way to support these ecosystem types vs. conversion traits!

I think we could make these available using https://docs.rs/http/0.2.4/http/struct.Extensions.html. So the syntax would look like

req.extensions_mut().get_mut::<RequestInitCfProperties>().unwrap().minify.javascript = true;

which is maybe a little verbose, but seems worth it to have ecosystem compatibility.

@jyn514
Copy link
Contributor Author

jyn514 commented Aug 25, 2021

We could make it less verbose by adding an extension trait:

use worker::RequestExt;
req.cf().minify.javascript = true;

which just calls extensions_mut().get_mut().unwrap() under the hood.

ByteAlex pushed a commit to ByteAlex/workers-rs that referenced this issue Apr 8, 2022
…ad of triggering Worker Threw Exception (cloudflare#13)

* add log_error attribute

* added console logging and renamed attribute

Co-authored-by: Leo Orshansky <lorshansky@cloudflare.com>
@michaelmerrill
Copy link

michaelmerrill commented Nov 29, 2022

Any update on this? Axum 6.0 has an example of how to use their router in a wasm environment, however, the router.call() accepts an http::Request. An example of how to translate a worker request to a http request would be very helpful.

@zebp
Copy link
Collaborator

zebp commented Nov 29, 2022

No real updates, there was a PR that worked on this but it wasn't merged due to some issues. I really like the idea of workers-rs reusing existing Rust http backend ecosystem but I'm not sure how well the JS and Rust sides of request/response will mesh. In JS the ownership of requests is very loose, I don't really know how things like Request::clone would work.

brainstorm added a commit to umccr/htsget-rs that referenced this issue Jan 25, 2023
@JorgeAtPaladin
Copy link

+1 for being able to use web frameworks such as axum.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants