feat: add configuration 'no-sys-proxy' to bypass the system proxy.

[Xu-Mj]

add command line configuration 'proxy-no-sys-proxy' and Trunk.toml configuration 'no-sys-proxy' to bypass system proxy. #689

[ctron]

Thanks for the PR. I think that's ok in general. What feels a bit weird is the multiplication of clients which are in there now. It feels like creating all possible options first, and then choosing later on is the wrong approach.

[Xu-Mj]

Thanks for the PR. I think that's ok in general. What feels a bit weird is the multiplication of clients which are in there now. It feels like creating all possible options first, and then choosing later on is the wrong approach.

Yes, I agree with that. I wrote below at first:

                let client = match (proxy.insecure, proxy.no_sys_proxy) {
                    (false, false) => {
                        state.client.clone()
                    }
                    (false, true ) => {
                        reqwest::ClientBuilder::new()
                            .no_proxy()
                            .http1_only()
                            .build()
                            .context("error building proxy client")?

                    }
                    (true, false ) => {state.insecure_client.clone()}
                    (true, true ) => {
                        reqwest::ClientBuilder::new()
                            .no_proxy()
                            .http1_only()
                            .danger_accept_invalid_certs(true)
                            .build()
                            .context("error building insecure proxy client")?
                    }
                };

This would simplify code elsewhere as well,
but I thought the clients should be centralized management.
So what do you think about the code above?
or if you give any suggestions would be great!

[ctron]

I am wondering if it wouldn't make more sense to:

1. Extract that into a single function first, something like create_client
2. Configuring the builder step by step:

   if no_proy {
     builder = builder.no_proxy();
   }

[Xu-Mj]

I am wondering if it wouldn't make more sense to:

1. Extract that into a single function first, something like create_client
2. Configuring the builder step by step:

   if no_proy {
     builder = builder.no_proxy();
   }

Great point. but there's a problem:
if we create clients at spwan_server() method, we don't know how many clients router() method does need;
if we create clients at router() method, it could be much more clients than 4, because users maybe need many proxies with different configures.

[ctron]

if we create clients at router() method, it could be much more clients than 4, because users maybe need many proxies with different configures.

The question is: is that actually a problem? How many proxies would one configure? 5? 10? 100? I would assume that even with 100, we should have no problem creating so many clients.

If you want to reduce it to an absolute limit, the you can come up with some "client factory". With an API like this:

#[derive(Clone, Debug, PartialEq, Eq, Hash]
struct ClientOptions {
 // … all kinds of options
}

struct ClientFactory {
  clients: HashMap<ClientOptions, reqwest::Client>,
}

impl ClientFactory {
  pub fn get(&mut self, options: ClientOptions) -> reqwest::Client { … }
}

[Xu-Mj]

Okay, I got your point eventually....
and I chose the easy way.

[ctron]

Looks good @Xu-Mj. Is that PR still a draft?

[Xu-Mj]

sorry about format errors, my ide format code automaticlly.