WorkerRequest/WorkerResponse traits

[dakom]

Opening this more as a discussion point, though the PR is fully baked afaict and ready to be merged if the idea is sound.

Motivation: it seems to me that there's currently a few conflicting ideas at play:

1. The underlying engine wants to deal with things in terms of web_sys
2. The original crate types were concrete types that had impls to/from web_sys
3. There's a move towards the http types, but these are completely generic over the Body and cannot be converted to web_sys (since there's no telling what that generic is)
4. There's a strong desire by many people to use axum, which imposes a specific concrete inner type - this can be converted to web_sys - but not everybody shares that desire, and workers-rs shouldn't be opinionated on this (just as the JS package doesn't enforce a specific web server framework) ... should be supported but not required.
5. Point 4 is putting to much pressure on the API imho... for example, simply enabling thehttp feature forces adding dep:axum too, even though they are theoretically decoupled.

The idea of the change in this PR is to instead nip the problem at its core, there only needs to be a single WorkerRequest and WorkerResponse traits, which go from and to web_sys respectively.

Out of the box, this trait is implemented for:

• web_sys types (so these can be used natively with no performance overhead)
• current worker Request/Response types (to maintain backwards compatibility until those are phased out)
• axum-flavored http types if the http feature is enabled

I've also added a proof of concept in the examples directory showing that this also allows downstream users to use any Request/Response type they want, just need to impl the traits.

If approved and merged, I don't think anything would break, and this paves the way for solidifying what the http API looks like... for that, I'd suggest next steps:

1. Completely remove the http feature - instead call it what it really seems to be, the axum feature, and doing this should enable axum to work as smoothly as possible out of the box
2. Completely feature-gate the current Request/Response types (they're very nice but not necessarily needed, users should be able to opt-out if they want to stick with web_sys or use axum, either direction)
3. In the examples, use whatever type is most convenient (maybe move the current Request/Response types over, or plain web_sys, or axum.. whatever!)

[avsaase]

Works like a charm!

One notable difference with #527 is that this PR requires the error type to implement Display instead of Error. Only Display is strictly necessary so I think it's fine but I wanted to point it out anyway.

[spigaz]

You have very good points, probably it might make sense to split into different packages, to try to make things clearer for users

• worker
• worker-http
• worker-axum
• worker-legacy

The point of the http feature was to be temporary, now we are discussing something more permanent.

My only issue, is that I believe we should try to maintain http, as much as possible, as there are other libraries targeting that crate.

I'm not even sure that axum is best suited for the current worker model, and most likely other frameworks will soon follow, now that http support is out there...
Its what your position regarding workers, applied also to the http layer.

[dakom]

it might make sense to split into different packages

Maybe! I'm not so familiar with how Rustaceans tend to approach this (separate packages vs. feature-gating). Either way makes sense to me.

My opinionated vote would be:

1. core code should not depend on any of these, it should talk only in terms of these traits, be totally implementation-agnostic, and if new methods (like get/set headers, method, etc.) are needed in core, they would be added to the traits

2. yes maintain a separate worker-http package that is not needed at all by core. Here it has feature-gated support for legacy, native, and axum. These should be properly additive (i.e. it's just the trait impls, no type aliasing to some shared name)

3. tests, examples, and userland code import both worker and worker-http

4. for convenience, worker should import and export worker-http, and pass through the same features. This is purely a convenience so users only import one package. If no http features are enabled, it shouldn't export any of them at all

Afaict, if a path like this is approved, it isn't blocked by the PR as it stands now, could build on top of it

[spigaz]

I believe it always depends on the crate's maintainers, although small, specific crates seem to be a common practice.

A good rule of thumb seems to be if its an extension or a layer on top.

2 -> I believe they should be separate crates, even probably also D1, R2, AI.
The modularity will also promote community crates.

3 -> Tests should be specific and internal for each crate, as they should be as self contained as possible. Using a layered approach you can even unit test them without hitting the workerd, this makes it easier to simulate error conditions, for instance.
But yes integration testing is always recommended also.

4 -> Normally its the upper layer that re-exports the lower layer.

[kflansburg]

I think it makes a lot of sense to use this trait based approach, where http is just one option we support. I think it could also split http functionality into a new crate and make it additive.

As far as the other suggestions, I'm a little worried about bikeshedding / making drastic changes here. I would rather focus on implementing missing APIs and make small modularity improvements over time.

[caiges]

FWIW, I tried your fork on one of our larger projects with success.

Is there anything left to do here?

[kflansburg]

Of the top of my head I would like to finish the swing on more generic http Body support, more generic Error type support, and possibly better errors from the macro when these traits do not line up.

Are you using web_sys::Response as well or do you have a different use case?

[dakom]

@kflansburg didn't quite follow, are you asking for more work to be done on this PR for "more generic [...] support"? If so, feel free to direct me to what you'd like to see done here (if it's work outside of the scope of this PR, then I suppose someone else like yourself would take it)

[kflansburg]

I suppose error types and macro errors can be landed in a separate PR, I was hoping to do it all in one release to minimize disruption. For now, I think there is one place this PR could be made slightly more generic.

[kflansburg]

Can this be more generic?

Suggested change

	impl IntoResponse for crate::HttpResponse {
	impl IntoResponse for http::Response<http_body::Body> {

[caiges]

What's the thought on impl for the concrete types rather than the alias?

[dakom]

@kflansburg made more generic in 370af17 (as far as crate::http::response::to_wasm() allows: impl http_body::Body<Data = Bytes> + 'static)

These seems to cover/conflict with the concrete axum impl too (so I removed that) :)

[kflansburg]

What's the thought on impl for the concrete types rather than the alias?

I don't understand the question. http_body::Body is a trait and will be more generic than the type alias.

[caiges]

Oh you're right, I just read that as http::body::Body which isn't a trait.