Add a new `connectionFactory` property to `HttpClient`

[brianquinlan]

Change

I propose that we add a new connectionFactory property to HttpClient with a signature like:

void connectionFactory=(
  Future<ConnectionTask<Socket>> f(
    Uri url, String? proxyHost, int? proxyPort)?
)

This would allow users to customize socket creation (e.g. for communication with dockerd/snapd over Unix domain sockets). Full details in the design doc.

The non-breaking alternative would be to make connectionFactory an argument to the HttpClient constructor.

Rationale

The HttpClient API already has multiple attributes that accept functions e.g.

void findProxy=(
  String f(Uri url)?)

void badCertificateCallback=(
  bool callback(
    X509Certificate cert,
    String host,
    int port
  )?
)

and implementing this as a constructor argument would be inconsistent with the rest of the API. Also, parameterizing all future functionality as constructor arguments does not seem scalable.

Impact

All classes that implements HttpClient (without extends Mock or equivalent noSuchMethod implementation) will need to be updated.

I suspect that there are not many implementations of HttpClient outside of mocks.

Mitigation

Users must implement the connectionFactory property.

[kevmoo]

Why not add another, optional, constructor param that takes the factory? Do we expect users to want to change this over the lifetime of an instance?

CC @natebosch

[brianquinlan]

@kevmoo I suggested that in my description i.e. the non-breaking alternative would be to make 'connectionFactory' an argument to the HttpClient constructor.

I'm fine with doing that but, if we always use that reasoning, all future parameters will be added as constructor arguments. That will:

1. make for inconsistent APIs e.g. why is findProxy a property and connectionFactory a constructor argument?
2. make constructors grow larger as an API evolves

If we decide that we can't make breaking changes by adding properties, then we should probably avoid using them, to the extent possible, when design new APIs because they will look odd when future functionality is added as constructor arguments.

[kevmoo]

Lots of named, optional parameters are fine, IMHO. Look at any Flutter API.

I think we should avoid foolish inconsistencies here. Having these be in the constructor (we don't even have to expose the field then) is cleaner and more idiomatic IMHO.

...UNLESS we have a reasonable use case where we want to be able to change the function during lifetime.

Curious what @natebosch @lrhn and/or @jakemac53 have to say

[brianquinlan]

@kevmoo fair enough

I don't think that users will need to be able to change the function - and, if they really need to, they can always use a conditional in the function that they assign.

[brianquinlan]

One interesting complication of adding a new HttpClient constructor argument is do I also add a new argument to HttpOverrides.createHttpClient?

Because, if I do, that will also break code.

If I don't then what's the rationale for having a securityContext argument but not connectionFactory?

[natebosch]

My initial inclination is to add this to the constructor, but I agree that it's a problem to have an inconsistency with HttpOverrides.createHttpClient.

[brianquinlan]

At least in Google code, there is at least an order of magnitude more definitions of createHttpClient than implements HttpClient (without extends Mock or equivalent noSuchMethod implementation).

[brianquinlan]

So how do we come to a decision here? I think that we have discussed 3 possibilities:

1. add a new connectionFactory property to HttpClient - breaks all code that implements HttpClient (without extends Mock or equivalent noSuchMethod implementation): there are not many such cases at Google
2. add a new connectionFactory argument to the HttpClient constructor and to HttpOverrides.createHttpClient - breaks all code that implements createHttpClient: there are many such cases at Google
3. add a new connectionFactory argument to the HttpClient but do not modify HttpOverrides - very compatible but makes the createHttpClient inconsistent with the actual HttpClient constructor

I prefer (1) because it makes for a consistent API with a reasonably low probability of significant breakage.

But I'm happy to do any of these.

[brianquinlan]

@natebosch @kevmoo Could you provide your opinions regarding #47887 (comment) ?

[natebosch]

@lrhn - WDYT?

I'd lean towards 3, but none of the options are very attractive.

[brianquinlan]

If we go with (3), will we never update HttpOverrides.createHttpClient as we add new HttpClient constructor arguments? So essentially the HttpOverrides API will match the behavior of HttpClient in 2017 forever?

I'm asking because I'm trying to understand our overall strategy regarding API evolution.