algesten / ureq

A simple, safe HTTP client
Apache License 2.0
1.72k stars 177 forks source link

ureq3 query parameter accessors #834

Closed asibahi closed 1 month ago

asibahi commented 1 month ago

Hello,

Following the announcement of ureq 3 yesterday I tried changing my project to it, and I quickly hit a stumbling block.

I use code like the following a lot in my code. Where I use the builder pattern with set and query doing their thing. This API is much of the reason I actually prefer ureq to other crates, as it is much simpler to build URLs this way.

    let access_token = AGENT
        .post("https://oauth.battle.net/token")
        .set("Authorization", &format!("Basic {creds}"))
        .query("grant_type", "client_credentials")
        .call()?
        .into_json::<AccessToken>()?;

Compare this to the same piece of code when I tried to use isahc

    let link = url::Url::parse_with_params(
        "https://oauth.battle.net/token",
        &[("grant_type", "client_credentials")],
    )?;

    let access_token = CLIENT
        .send(
            isahc::Request::post(link.as_str())
                .header("Authorization", &format!("Basic {creds}"))
                .body(())?,
        )?
        .json::<AccessToken>()?;

Where I also depend on a url crate to get a semblance of a builder pattern.

A migration guide that goes over things like this or at least adding some useful extension traits to http types would go a long way of matching the ergonomics of ureq 2.

Final note: It is not clear to me, as a user, what the benefits of switching to 3 are. What do I miss out on if I stick to 2 ?

algesten commented 1 month ago

Excellent feedback. Thanks!

The query feature could certainly be brought back.

Final note: It is not clear to me, as a user, what the benefits of switching to 3 are. What do I miss out on if I stick to 2 ?

I'm only going to keep 2 alive for so long. The code for that has hit a bit of a dead end in terms of maintainability. My ambition is to convince everyone 3 is the better option.

algesten commented 1 month ago

@asibahi this is solved in main now, right?

asibahi commented 1 month ago

Hey @algesten thanks for the addition. The transition proved generally smooth for my small code base.

Notes on the other .. er .. speed bumps of the transition:

  1. Reading the body of a response into json changed to be a bit more boiler platey, I assume this is by design.
// from
let res = req.call()?.into_json::<AccessToken>()?;

// to
let res = req.call()?.body_mut().read_json::<AccessToken>()?;
  1. No call() method on post requests. Did send(&[]) instead. Also I assume this is by design?

  2. Creating an Agent with configuration became more awkward. I saw similar feedback on reddit.

// from
let agent = ureq::AgentBuilder::new()
        .timeout_connect(std::time::Duration::from_secs(2))
        .user_agent("text")
        .build()

// to
let mut config = ureq::Config::new();
config.timeouts.connect = Some(std::time::Duration::from_secs(2));
config.user_agent = Some("text".into());

let agent = config.into()

Overall though the transition was simply changing the dependency and following the errors and warnings as marked, with looking up the documentation as needed. Overall smooth experience and for the most part it was possible to simply do a search and replace across the whole code base.

algesten commented 1 month ago

@asibahi thank you for this feedback!

  1. Reading the body of a response into json changed to be a bit more boiler platey, I assume this is by design.

Yes, this is inevitable when moving to the http-crate based API, where we must use body_mut() to get the body. On balance, I prefer using the http-crate though to have a familiar API.

  1. No call() method on post requests. Did send(&[]) instead. Also I assume this is by design?

It is. Body-less POST are a bit of an oddball, and on balance I don't think it's wrong to force the use of empty. Could be we should add a convenience send_empty() to reinforce it. What do you think?

  1. Creating an Agent with configuration became more awkward. I saw similar feedback on reddit.

Yeah, I think I will bring the builder back.

asibahi commented 1 month ago

Yeah I think a send_empty() function would probable reflect the API design intention better.