Switch to `http` crate by kflansburg · Pull Request #474 · cloudflare/workers-rs

kflansburg · 2024-03-11T15:11:24Z

No description provided.

* Bump wasm-streams * Use Body::into_readable_stream * fmt * clippy * Remove unused js hack * clippy * Added remaining routes and fixed testes (#473) * Added test script to package.json. * Added worker-sandbox r2 routes. * Added worker-build to the npm test script. * Added worker-sandbox r2 routes - cleanup. * Added worker-sandbox queue routes. * Added worker-sandbox service-bindings routes - implied porting and adapting some improvements from the latest version. * Added worker-sandbox service-bindings routes - implied porting and adapting some improvements from the latest version - fmt. * Added worker-sandbox cache stream route - Implied porting some more recent stuff. * Added worker-sandbox remaining cache routes. * Added worker-sandbox init called route. * Added worker-sandbox custom-response-body route. * Added worker-sandbox now route. * Added worker-sandbox redirect-307 route. * Added worker-sandbox redirect-default route. * Added worker-sandbox subrequests routes and fetch helpers that were requeried. * Added worker-sandbox subrequests fetch-timeout to be shorter. * Added worker-sandbox request catchall fixed test case. * Added worker-sandbox request xor route. * Added worker-sandbox request xor route. * Added worker-sandbox websocket route. * worker-sandbox queue routes clean up. * worker unused dependencies clean up. * worker-sandbox: Added missing d1 routes and improved d1 tests to use miniflare. * worker-sandbox: Clippy. * worker-sandbox: fmt. --------- Co-authored-by: Alexandre Faria <alexandre.faria@auroka.pt>

kflansburg · 2024-03-11T19:41:18Z

@spigaz I've merged main into this branch.

Looks like I broke 4 tests that I'm working on now.

I have to say, there are a lot of breaking changes here. I'm thinking:

Review changes to public API (and removed APIs)
Update README and docs with new APIs and recommended patterns
Probably release as 0.1.0 to signal a substantial API change.

Overall I'm not feeling super comfortable about this.

@zebp do you have thoughts?

avsaase · 2024-03-11T19:57:31Z

Amazing work!

I would be very happy if this is merged and released. It will be a big breaking change but I think it'll be such a huge improvement to the usability of the worker-rs crate that it's definitely worth it. I'm saying that as a user who will have to update a worker with 100+ routes.

With semver, 0.1.x versions already indicate that there is no API stability so the fact that the crate currently has a 0.0.x version should've been enough indication to end users that some churn was to be expected.

I'm happy to assist with updating the documentation if that helps (UPDATE: I'm working on an example now).

avsaase · 2024-03-11T20:03:09Z

Another reason to go forward with this is that it'll supersede at least 5 open PR's.

jakubadamw · 2024-03-11T20:28:30Z

Cargo.toml

 futures-channel = "0.3.29"
 futures-util = { version = "0.3.29", default-features = false }
-http = "1.0.0"
+http = "0.2.0"


I am curious about this change. What's the rationale? The ecosystem has been slowly moving over to http 1.0. Is there a reason to go back in this crate's case?

@jakubadamw This is work in progress, most likely its just some part of the merge that didn't go well.

And that one is easy to find and fix. Thanks!

The version of the fork was way behind the main, so it was always a likely painful process.

Yep, there were a number of crates using 0.2 as a dep that would have needed major version bumps to move to 1.0. I can try to do that as part of this PR, but I was mostly just trying to get the merge done.

Ah, I see, fair enough!

spigaz · 2024-03-11T20:32:35Z

@kflansburg Thanks! I feel exactly the same.

This was always going to be disruptive, and I do expect a lot regressions, as we have only about 50 tests, so I was counting with their interest to help flush them out and to write some tests if necessary, that's what pushed me in the end.

You can always create a stable branch, but we survived many months without a release.

The sooner you push it out, the sooner the issues will start to flush in.

In the end its what all of us want.

So a breaking api and a warning should be enough.

I can pick up if you want.

kflansburg · 2024-03-11T20:42:03Z

@spigaz were you able to get the D1 dump test to run? I'm having trouble and I see in our docs that it is not supported for databases created after alpha, so I may not be able to reproduce locally.

spigaz · 2024-03-11T21:02:43Z

@kflansburg Yesterday it was running fine, now I'm having the same issue.

The test case is executed by miniflare so it might be more permissive.

Unfortunately I deleted the previous branch, but I'm going to try to sort this out.

kflansburg · 2024-03-11T21:23:33Z

@kflansburg Yesterday it was running fine, now I'm having the same issue.

The test case is executed by miniflare so it might be more permissive.

Unfortunately I deleted the previous branch, but I'm going to try to sort this out.

Yep, it works from before I merged main. So far I've tried downgrading miniflare and removing the fetch mock. Not sure what else changed :/

jakubadamw · 2024-03-11T21:32:03Z

The test case is executed by miniflare so it might be more permissive.

Unfortunately I deleted the previous branch, but I'm going to try to sort this out.

You may still find it in your git reflog. 🙂

avsaase · 2024-03-11T22:24:51Z

I'm creating an example to demonstrate how to use this with the axum (0.6) router but I'm running into the issue that there's no convenient way to convert a worker::body::Body to a hyper::body::Body which is required for the call call:

use axum::{http::StatusCode, routing::get, Router};
use tower_service::Service;
use worker::{event, Body, Context, Env, Request, Response};

fn router() -> Router {
    Router::new().route("/", get(root))
}

#[event(fetch)]
async fn fetch(req: Request, env: Env, _ctx: Context) -> worker::Result<Response> {
    console_error_panic_hook::set_once();

    let response = router().route("/", get(root)).call(req).await.unwrap();

    Ok(response.map(Body::new))
}

pub async fn root() -> StatusCode {
    StatusCode::OK
}

I'm not entirely sure why this is a problem though because worker::body::Body does implement the required HttpBody trait. I'll investigate further tomorrow.

Here's the full example if anyone is interested: https://github.com/avsaase/workers-rs/tree/axum-router-example/examples/axum

kflansburg · 2024-03-11T23:30:37Z

Ok, so the main error message is being generated here: https://github.com/cloudflare/workerd/blob/b53050ed2117ae10d2b673cb36d7b137905a1549/src/cloudflare/internal/d1-api.ts#L81

Which happens when workerd gets a 404 back from miniflare, which happens here: https://github.com/cloudflare/workers-sdk/blob/aabae0f6a0414262ee85d7a2540f01179ae86f20/packages/miniflare/src/workers/shared/router.worker.ts#L47

This version of miniflare / workerd is working for the dump method on main, but I cannot figure out where that route is configured or why its not being defined on this branch.

spigaz · 2024-03-11T23:31:33Z

@jakubadamw Unfortunately I removed the directory, but github saved the PR, dahh...

Once I had the two versions, running side by side... Only a couple of minutes...

@kflansburg It was a miniflare dependency issue, it seems that an older version works differently.

I already asked on discord to see what is the proper behavior, but if it was dropped, then we should remove not only the test case, but also the method.

I created a #476 with it running just in case its helpful.

spigaz · 2024-03-11T23:51:59Z

Ok, so the main error message is being generated here: https://github.com/cloudflare/workerd/blob/b53050ed2117ae10d2b673cb36d7b137905a1549/src/cloudflare/internal/d1-api.ts#L81

The function has the comment:

  // DEPRECATED, TO BE REMOVED WITH NEXT BREAKING CHANGE
  public async dump(): Promise<ArrayBuffer> {

As you mention the documentation says:
"This API only works on databases created during D1’s alpha period."
D1 is in public beta.
https://developers.cloudflare.com/d1/build-databases/query-databases/#await-dbdump

Even without @mrbbot confirmation I'm guessing he dropped support for dump on purpose, that explains why the more recent versions cause the error.

I'm guessing it makes sense to drop it also, I just wonder if they have some alternative planned.

kflansburg · 2024-03-12T00:07:09Z

@spigaz thanks for finding that. What threw me off is that the file was moved when the route was removed, so I didn't see it when scanning the history for the file. cloudflare/workers-sdk@88d4e45

Ok, I'm inclined to remove this. I think d1 tests may actually be disabled on main so would be good to re-enabled them.

workers-rs/worker-sandbox/tests/d1.spec.ts

Line 18 in 7ed61c0

describe.skipIf(!hasLocalDevServer)("d1", () => {

spigaz · 2024-03-12T00:22:32Z

@kflansburg I just got a confirmation from @geelen: "dump only applied to alpha DBs, it's being replaced with export for beta ones but that's not available yet"

And from @mrbbot:
"miniflare@3.20230918.0 and above do not support D1 dump. This version re-implemented simulators using Durable Objects: https://github.com/cloudflare/miniflare/releases/tag/v3.20230918.0. Versions below that should support it."

I'm not sure how the d1 tests were executed, they weren't running locally, but now using miniflare they should be all just fine, as long as we drop dump.

spigaz · 2024-03-12T01:01:45Z

@kflansburg It seems the http or http-body update will require a reimplementation of some parts.

How do you want to proceed?

I already discussed with @mrbbot about adding support for workerd to wasm-bindgen-test and miniflare-rs, it seems I can start working on that.

kflansburg · 2024-03-12T02:27:41Z

@kflansburg It seems the http or http-body update will require a reimplementation of some parts.

How do you want to proceed?

I already discussed with @mrbbot about adding support for workerd to wasm-bindgen-test and miniflare-rs, it seems I can start working on that.

Is this referring to the update to http 1.0? I think I would like to do that in a follow on PR if that is ok. This is already daunting as it it.

spigaz · 2024-03-12T11:31:00Z

@kflansburg I understand.

To be honest given the implications of that update, plus a merge with a branch that forked about a year ago...

I'm not sure the wisest option isn't to start with main, updating to the latest http and http-body and then back-porting only the required changes.

I say this also because I noticed API changes, for instance on fetch and I wasn't sure what was the "newer" version the one on main or the one on the "http-types branch".

Once I read somewhere about they having an internal document about this, an internal discussion, but that's as far as I know about that, so they should know when reviewing.

kflansburg · 2024-03-12T13:50:28Z

@kflansburg I understand.

To be honest given the implications of that update, plus a merge with a branch that forked about a year ago...

I'm not sure the wisest option isn't to start with main, updating to the latest http and http-body and then back-porting only the required changes.

I say this also because I noticed API changes, for instance on fetch and I wasn't sure what was the "newer" version the one on main or the one on the "http-types branch".

Once I read somewhere about they having an internal document about this, an internal discussion, but that's as far as I know about that, so they should know when reviewing.

I'm not aware of any internal docs / discussion. But I agree, it is tempting to try to implement this off of main in a much more surgical way. As it stands this PR has had 3 or 4 distinct owners and so a lot of context has been lost.

I will probably try to give this a brief shot today to see how it could be done.

Kakapio · 2024-03-12T14:06:39Z

@kflansburg Thanks! I feel exactly the same.

This was always going to be disruptive, and I do expect a lot regressions, as we have only about 50 tests, so I was counting with their interest to help flush them out and to write some tests if necessary, that's what pushed me in the end.

You can always create a stable branch, but we survived many months without a release.

The sooner you push it out, the sooner the issues will start to flush in.

In the end its what all of us want.

So a breaking api and a warning should be enough.

I can pick up if you want.

Would be more than happy to write tests for this if you can specify what needs coverage.

spigaz · 2024-03-12T16:24:02Z

Once I read somewhere about they having an internal document about this, an internal discussion, but that's as far as I know about that, so they should know when reviewing.

I'm not aware of any internal docs / discussion. But I agree, it is tempting to try to implement this off of main in a much more surgical way. As it stands this PR has had 3 or 4 distinct owners and so a lot of context has been lost.

I will probably try to give this a brief shot today to see how it could be done.

@SebastiaanYN or @zebp Can you share if that has any relevance on @kflansburg work?
I guess it was one of you that mentioned it.

@kflansburg I used that strategy to get the tests running and ported what was implied.

I normally have have multiple folders, multiple vscodes and a visual diff comparing the folders.

kflansburg · 2024-03-12T17:14:16Z

@spigaz what do you think of something like this (only a skeleton, lots of todo!)?

https://github.com/cloudflare/workers-rs/pull/477/files

The idea here is to introduce http as a feature flag and take a more incremental approach.

For now, the flag would change the arguments / return type of the event(fetch) handler, and could probably also change the behavior of Fetcher, but we would refrain from rewriting all of the APIs to use http throughout.

We would also provide From implementations so that users could easily convert to / from the new and old request and response types, so that they could incrementally convert their code.

spigaz · 2024-03-12T17:33:01Z

The idea here is to introduce http as a feature flag and take a more incremental approach.

@kflansburg I agree, I think its a brilliant idea.

That will allow us to move forward with small PRs without causing instability, while at the same time you can accept PRs for other stuff.

And to be honest, I definitely agree on the part of changing the least possible, specially while we have few tests. As a rule we should only change when we have enough tests.

The approach regarding testing is also awesome, because it will allow us to focus on API compatibility.

In the long run it might seem more work, but I believe it will be worth it.

Let me know if you want me to start filling the blanks.

kflansburg · 2024-03-12T17:52:47Z

The idea here is to introduce http as a feature flag and take a more incremental approach.

@kflansburg I agree, I think its a brilliant idea.

That will allow us to move forward with small PRs without causing instability, while at the same time you can accept PRs for other stuff.

And to be honest, I definitely agree on the part of changing the least possible, specially while we have few tests. As a rule we should only change when we have enough tests.

The approach regarding testing is also awesome, because it will allow us to focus on API compatibility.

In the long run it might seem more work, but I believe it will be worth it.

Let me know if you want me to start filling the blanks.

Ok, cool. I've also added the changes I mentioned to Fetcher. I will take a stab at filling in some of the blanks this afternoon.

kflansburg · 2024-03-13T21:12:58Z

Closed in favor of #477

SebastiaanYN and others added 12 commits June 15, 2023 11:44

Switch to using http::Request and http::Response

d25a89f

Make types and futures Send and Sync

9898ba9

Directly get JS body as ArrayBuffer when possible

21a8f37

Don't set response body if it's empty

e2fbacc

Provide clone functions for request and response types

43483b8

Make cache API accept both web_sys and http Request/Response

e28c7c0

Add request redirect and cf properties back

f6b8925

Add .text() and .json() methods to Body

2518c3d

Add documentation

ecf2321

wip

39b3701

Merge main

343bddc

kflansburg mentioned this pull request Mar 11, 2024

Switch to using http::Request and http::Response #286

Closed

3 tasks

jakubadamw reviewed Mar 11, 2024

View reviewed changes

kflansburg force-pushed the zeb/http-types branch from efe9fa7 to d47e476 Compare March 11, 2024 20:48

fix some tests

2b2e155

kflansburg force-pushed the zeb/http-types branch from d47e476 to 2b2e155 Compare March 11, 2024 21:12

Remove d1 dump test

990d1fc

kflansburg closed this Mar 13, 2024

finalyards mentioned this pull request Jan 12, 2026

[Feature] Use standardized Rust crate defintions for better interoparability with other Rust crates #307

Closed

1 task

Conversation

kflansburg commented Mar 11, 2024

Uh oh!

kflansburg commented Mar 11, 2024

Uh oh!

avsaase commented Mar 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

avsaase commented Mar 11, 2024

Uh oh!

jakubadamw Mar 11, 2024

Choose a reason for hiding this comment

Uh oh!

spigaz Mar 11, 2024

Choose a reason for hiding this comment

Uh oh!

kflansburg Mar 11, 2024

Choose a reason for hiding this comment

Uh oh!

jakubadamw Mar 11, 2024

Choose a reason for hiding this comment

Uh oh!

spigaz commented Mar 11, 2024

Uh oh!

kflansburg commented Mar 11, 2024

Uh oh!

spigaz commented Mar 11, 2024

Uh oh!

kflansburg commented Mar 11, 2024

Uh oh!

jakubadamw commented Mar 11, 2024

Uh oh!

avsaase commented Mar 11, 2024

Uh oh!

kflansburg commented Mar 11, 2024

Uh oh!

spigaz commented Mar 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spigaz commented Mar 11, 2024

Uh oh!

kflansburg commented Mar 12, 2024

Uh oh!

spigaz commented Mar 12, 2024

Uh oh!

spigaz commented Mar 12, 2024

Uh oh!

kflansburg commented Mar 12, 2024

Uh oh!

spigaz commented Mar 12, 2024

Uh oh!

kflansburg commented Mar 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kakapio commented Mar 12, 2024

Uh oh!

spigaz commented Mar 12, 2024

Uh oh!

kflansburg commented Mar 12, 2024

Uh oh!

spigaz commented Mar 12, 2024

Uh oh!

kflansburg commented Mar 12, 2024

Uh oh!

kflansburg commented Mar 13, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

avsaase commented Mar 11, 2024 •

edited

Loading

spigaz commented Mar 11, 2024 •

edited

Loading

kflansburg commented Mar 12, 2024 •

edited

Loading