Consider alternate chaining pattern for DI/builder #5572
Description
(Written by @SteveSandersonMS; I'm copying this into the repo from an offline location...)
Now
Currently the API looks like:
services.AddChatClient(pipeline => pipeline
.UseDistributedCache()
.UseFunctionCalling()
.Use(new SomeConcreteClient()); // Or pass a factory (Func<IServiceProvider, IChatClient>)
... and the proposed Aspire+MEAI integration looks like:
services.AddOpenAIChatClient("service-name", pipeline => pipeline
.UseDistributedCache()
.UseFunctionCalling());
Possibility
However, we could change AddChatClient
so that it actually returns the ChatClientBuilder
instead of the IServiceCollection
. Then it would look like:
services.AddChatClient(new SomeConcreteClient()) // Or pass a factory (Func<IServiceProvider, IChatClient>)
.UseDistributedCache()
.UseFunctionCalling();
... and the proposed Aspire+MEAI integration would become:
services.AddOpenAIChatClient("service-name")
.UseDistributedCache()
.UseFunctionCalling();
In my opinion this looks a lot better, as it removes the lambda, nested brackets, and the meaningless-looking pipeline => pipeline
that hangs at the end of the first line.
This would be a legitimate pattern for DI with plenty of prior art of AddX
calls that return a builder you can chain further setup steps on.
Challenges
- We'd have to change
ChatClientBuilder
so that, instead of acceptingIServiceProvider
as a constructor arg and exposing it as a property, it would have to work without access to anIServiceProvider
until the DI service is resolved.- Mostly this is fine because the
Use(Func<ChatClientBuilder, ChatClientBuilder>)
method already has an overload that takesFunc<IServiceProvider, ChatClientBuilder, ChatClientBuilder>
and so that would become the only overload - The one thing that would have to change is its
Use(IChatClient)
that terminates the pipeline. This would have to change toUse(Func<IServiceProvider, IChatClient>)
. And we should probably rename it toUseInnerClient
or back toBuild
otherwise it gets really hard to differentiate.- Alternatively we remove that method entirely and change the constructor to have two overloads, one accepting
IChatClient
and the other acceptingFunc<IServiceProvider, IChatClient>
. Then the usage pattern is that you pass the inner client (or factory for one) to the constructor, and anyUse
calls are assumed to come before that in the pipeline.
- Alternatively we remove that method entirely and change the constructor to have two overloads, one accepting
- Mostly this is fine because the
- It becomes harder for us to detect whether you've configured a pipeline or not. Today we can check if the
pipeline
func is null, and if so, have a default pipeline.- We could do something slightly trickier where
ChatClientBuilder
keeps track of whether anyUse
calls have been made, and then at service resolution time, we apply a default pipeline if not - Or we move away from the concept of a default pipeline, which is dubious anyway because it's weird that adding a pipeline phase erases the default pipeline you had before
- We could do something slightly trickier where
Altogether I think the improved aesthetics of doing this without pipeline => pipeline
make it worthwhile to pursue.