Support Dependency Injection #44
Conversation
this is a breaking change, it is not backwards compatible closes OkGoDoIt#41
responses should be disposed, but as this returns the response it will dispose twice
|
Wow @KeithHenry, thank you for putting the effort into implementing this, it's no small feat! 🤩 I have concerns about adding DI, both compatibility and ease-of use. I notice the PR eliminates support for .NET Standard and therefore the .NET Framework. It also adds several Nuget Package dependencies. It feels optimized for Asp.net rather than broad usage. Is DI widely used outside of the asp.net use case? I want to ensure this still works for Unity, Mobile, WinForms, etc. Can you include updated readme examples? I would want to ensure the simple use cases are still easy to achieve, especially for the majority of .Net developers who may be unfamiliar with DI. I have to admit I'm not that familiar with DI in .Net, so I may be a bit biased against it, but I'm willing to be convinced. I just want to make sure we aren't adding complications for 90% of casual users to accommodate the use cases of 10% of enterprise users. |
|
it would be nice if we could support both, but i think DI is the way things are going regardless. i can't speak to all these platforms, but i have used it in console apps. i know this kindof pushes the legacy users out though. would be great if both options were available through separate endpoints. https://learn.microsoft.com/en-us/dotnet/core/extensions/dependency-injection |
| public OpenAIAPI(HttpClient httpClient) : this() => | ||
| this.Client = httpClient; | ||
|
|
||
| /// <summary> | ||
| /// Creates a new entry point to the OpenAPI API, handling auth and allowing access to the various API endpoints | ||
| /// </summary> | ||
| /// <param name="auth">The authentication details for the API</param> | ||
| [Obsolete(""" | ||
| This constructor will generate a new HTTP client every time it is called. | ||
| Do not use this in scenarios where multiple instances of the API are required. | ||
| This is provided for backwards compatibility, use .NET Dependency Injection instead. | ||
| """, false)] | ||
| public OpenAIAPI(APIAuthentication auth) : | ||
| this(EndpointBase.ConfigureClient(new HttpClient(), auth)) { } | ||
|
|
||
| /// <summary> | ||
| /// Creates a new entry point to the OpenAPI API, handling auth and allowing access to the various API endpoints | ||
| /// </summary> | ||
| /// <param name="key">The authentication key for the API</param> | ||
| [Obsolete(""" | ||
| This constructor will generate a new HTTP client every time it is called. | ||
| Do not use this in scenarios where multiple instances of the API are required. | ||
| This is provided for backwards compatibility, use .NET Dependency Injection instead. | ||
| """, false)] | ||
| public OpenAIAPI(string key) : | ||
| this(EndpointBase.ConfigureClient(new HttpClient(), new APIAuthentication(key))) { } |
There was a problem hiding this comment.
An idea to give you the backwards compatibility, but it's somewhat messy - alternate constructors that create an HttpClient when called and configure it, but that match the old patterns.
These will cause warnings for users upgrading still.
|
@OkGoDoIt answers to your points
Yup, you could probably support The dependencies it adds are ones everyone using .NET5 or above already has
Since .NET 5 it's been the default for all apps.
I get that, but if I was doing this in my own time I'd just be using However, a casual user starting a new .NET app (console, web, whatever) in Visual Studio 2022 Community or with the |
| public static IServiceCollection AddOpenAIService(this IServiceCollection services, IConfiguration configuration) | ||
| { | ||
| var section = configuration.GetSection("openAI"); | ||
| if (!section.Exists()) return services; | ||
|
|
||
| string? key = section["key"]; | ||
| if (key is null) return services; | ||
|
|
||
| string? organisation = section["org"]; | ||
| return services.AddOpenAIService(new APIAuthentication(key, organisation)); | ||
| } |
There was a problem hiding this comment.
With introducing DI one should maybe also think of introducing the Options Pattern.
This will introduce a AddProjectServices(this IServiceCollection services, Action<ApiAuthenticationOptions> configure)
This way you don't have to pass down the IConfiguration and instead can work directly with the object in question.
Within your Startup/Program.cs you can then use the IConfiguration.GetSection().Bind to pass down the actual options. This way the library won't depend on the IConfiguration library and one can use IOptions<T> to fetch the apikey and such from controllers.
@KeithHenry You've made a great starting point but i think this can be improved even more, do you mind if i helped you out here/there?
I think the introduced packages are all compatible with .netstandard20 according to nuget.org So there's no need to bump the targetframework from netstandard2.0 to net7.0? It'd be nice if we can upgrade to .net7.0 since this allows us to use the latest C# language features, but this comes at the cost of dropping netframework support. Another possibility is to have the packages released for multiple targetframework; aka .net7.0 AND netstandard 2.0 |
|
I love the DI support, but it probably should be a separate package that adds DI support on top of the core. |
This library has a severe bug #41 - this PR fixes that, but can't keep backwards support. There's a possible workaround, but it would still make the DI version the main one. |
Breaking change: switch from inline instantiation to using .NET's dependency injection.
Current usage will no longer work:
The
OpenAIAPIconstructor is now internal, and takes a client generated by anHttpClientFactory(fixes #41)Instead in your startup register the service:
This will inject
IOpenAI, which you can access with[FromServices]orserviceProvider.GetService<IOpenAI>()Also fixes #42 and #43