Seanaye/feat/serverside http

[seanaye]

This PR adds bindings necessary for server side fetch api. This allows rust code to be deployed easily in v8 isolate environments like cloudflare workers or Deno Deploy

[ranile]

I don't understand the use case. Why do you need to manually construct Response?

[seanaye]

This is required for use in modern serverside js runtimes, currently there is no way to build a response to the client in gloo.

The below example uses raw web_sys since gloo is not supported

See https://github.com/seanaye/discord-bot/blob/main/main.ts

To run that example locally deno task dev

[seanaye]

I was just looking at the docs for http it looks like it would be possible to implement impl TryInto<web_sys::Response> for http::Response as an alternative

[seanaye]

I have a new proposed api which leverages try_into and try_from for Request and Response gonna mark as draft until I finish the docs and tests but the code is mostly done

[futursolo]

In general, I think this is a good change which gives requests a separate builder for builder pattern and requests becomes a more general reader / writer type.

However, I am a little bit unsure how this could actually be used by actual server-sided applications running on "edge" runtimes as I do not think users can avoid runtime specific binding for more complex functions as each runtime is different on how to access additional information that describes the function itself (commonly referred to as "Context") and how to access environment variables or secrets.

E.g.: Cloudflare Workers provide cf() method to access additional request information not provided by web_sys::Request and worker::Env to access environment variables and secrets.

[seanaye]

@futursolo the env variables can be passed in as second arguments to the handler function if required. I actually have a live demo of this code working on Deno Deploy if you want to see. I'm not passing any env vars but it would also be possible.

Live demo link: https://seanaye-discord-bot.deno.dev/
Source for demo: https://github.com/seanaye/discord-bot/blob/main/main.ts

Lambda's are not a fetch web standard based API. Deno Deploy doesnt really have a Context it does have environment variables but these can be passed as function arguments. All of the http request information is encapsulated in the standard Request object which is handed off to rust. It looks like cloudflare workers dont abide by the Request standard, but Deno does in Deno Deploy

[futursolo]

Lambda's are not a fetch web standard based API. Deno Deploy doesnt really have a Context it does have environment variables but these can be passed as function arguments.

According to the documentation, environment variables are accessed via Deno.env which is a runtime-specific API.
Information that describes the function itself is provided as environment variables as well.

https://deno.com/deploy/docs/projects#environment-variables

It looks like cloudflare workers dont abide by the Request standard, but Deno does in Deno Deploy

I do not think there is a "standard" of how Request and Response should be used if they are not paired with fetch. They are defined as input and results of the fetch() API on the Specification. Additional information of the request in Deno Deploy is provided as the second argument to the function as ConnInfo, which is runtime specific.

[seanaye]

ah I see what you mean. I still think this is much better for handling Request/Response on the server with wasm. I don't think there is a good solution for handling things like Deno.env outside of passing it to the handler as a second argument e.g.

serve((req, connInfo) => handler(req, Deno.env.get("MY_API_SECRET"), connInfo.remoteAddr.definition.transport))

#[wasm_bindgen]
pub async fn handler(req: web_sys::Resquest, secret: String, transportMethod: String) -> Result<web_sys::Response, JsError> {
...
}

These values are platform specific as you mentioned and should not be handled by gloo. The caller should be responsible for them. Its possible to injects the values into rust as other arguments

That being said 99% of applications dont need to use things like connInfo, most relevant info is available inside the Request standard object as defined in the spec

[seanaye]

Is there anything further you need from me to move this forward?

[futursolo]

I am sorry that I was in the process of switching to a new PC. So I wasn't able to review this pull request.

I have put in some suggestions. Hope we can get this merged soon.

[seanaye]

Thanks for the review, somehow didn't see it until now. Will probably get a chance to implement later this week

[seanaye]

@futursolo should be good to re-review / merge now

[futursolo]

Thank you.

I think the implementation looks good.

I have added some minor suggestions for documentation and I think it would be mergable after they are fixed.

I will also request @hamza1311 to have a look at this PR as they are the original author of this crate.

[futursolo]

Let's merge this one since I haven't heard back from @hamza1311 for 2 weeks.

There are some other pull requests also targeting this crate so I do not want to block this pull request and cause conflicts for the other pull request.

I do not see any big issues with this implementation and if there are any smaller issues, we can fix it at a later time.