Skip to content

Conversation

SpikeHD
Copy link

@SpikeHD SpikeHD commented Feb 28, 2025

Addresses #181

TODO

(These are just copied from the above issue :P)

  • Be part of the existing blitz-net crate with feature flags for each backend
  • Use the new 3.x version of ureq
  • Split the reqwest and ureq implementations in separate files/modules
  • Use a threadpool to allow for multiple concurrent requests
  • Implement the blitz_net::get_text method as well as blitz_net::Provider

@SpikeHD SpikeHD marked this pull request as draft February 28, 2025 20:32
@SpikeHD SpikeHD changed the title DRAFT: ureq backend for blitz-net Add ureq backend for blitz-net Feb 28, 2025
@SpikeHD
Copy link
Author

SpikeHD commented Feb 28, 2025

To test this, set ureq as the default feature in blitz-net, then change packages/blitz/src/lib.rs@L36 like so:

- let html = rt.block_on(blitz_net::get_text(&url));
+ let html = blitz_net::get_text(&url));

@SpikeHD
Copy link
Author

SpikeHD commented Feb 28, 2025

@nicoburns I know you'll be gone, but once you're back, I have a couple architecture questions:

  • Is there a better way to handle the two Providers? Right now they are almost the same, except for the fact that fetch_inner is async on the reqwest backend. I'd like to ofc prevent repeated code but my little brain can't conceptualize how that might look
  • How should the other packages be modified to not use tokio (in the case someone is gonna use the ureq backend to avoid an async runtime, for example)? From what I can tell, three packages use it, blitz, blitz-shell, and dioxus-native. Or is that out of scope for this PR (I would be very okay with this :P)?

"Allow edits by maintainers" should be on if you feel inspired haha

@SpikeHD SpikeHD marked this pull request as ready for review March 8, 2025 04:50
Copy link
Collaborator

@nicoburns nicoburns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for taking so long to review this. I've requested a couple of specific changes.

In regard to your questions:

  • I think you should be able to unify the interface of the backends by using a callback-based interface which should work for both async and threadpool+channel based backends.
  • Removing tokio from other crates has now happened :)

I think I'm probably going to try to land #188 before this as I suspect it will be easier to rework this one than that one.

Comment on lines +38 to +43
self.client
.run(<blitz_traits::net::Request as Into<http::Request<()>>>::into(request))?
} else {
self.client.run(<blitz_traits::net::Request as Into<
http::Request<Vec<u8>>,
>>::into(request))?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These type assertions are kinda horrible. I think you could make this much nicer by splitting the conversion onto a separate line:

let request : http::Request<()> = request.into();
self.client.run(request)?

I also wonder how useful the () path is here. Could we just always use the Vec<u8> version and pass an empty Vec if there's no data?

@@ -0,0 +1,16 @@
use blitz_traits::net::NetCallback;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move the callbacks into the "backend" modules (so all the code each backend is in the same place) please?

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

Successfully merging this pull request may close these issues.

2 participants