feat: Add Extension Method for adding global Hook via DependencyInjection #459
Conversation
Signed-off-by: Kyle Julian <38759683+kylejuliandev@users.noreply.github.com>
Signed-off-by: Kyle Julian <38759683+kylejuliandev@users.noreply.github.com>
| /// </summary> | ||
| /// <typeparam name="THook">The type of<see cref="Hook"/> to be added.</typeparam> | ||
| /// <param name="builder">The <see cref="OpenFeatureBuilder"/> instance.</param> | ||
| /// <param name="implementationFactory"></param> |
There was a problem hiding this comment.
Missing description for implementationFactory
| public static OpenFeatureBuilder AddHook<THook>(this OpenFeatureBuilder builder, Func<IServiceProvider, THook> implementationFactory) | ||
| where THook : Hook | ||
| { | ||
| var hookName = typeof(THook).Name; |
There was a problem hiding this comment.
FeatureProvider's have a name or metadata tag which we use as the key when injecting them into the DI container. Here we use the Hook class name. Maybe we want to use something else incase hooks clash?
There was a problem hiding this comment.
They are being added as singleton. So if the hook already exists in the container should we really add them again? I would vote to do a continue and ignore adding more instances.
There was a problem hiding this comment.
More that if two hooks have the same class name, then only one will actually be injected into the DI container
Not sure if there are many hooks out there, and whether consumers would use both hooks. Not sure if we should add a caution or warning about it in the xml comment
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #459 +/- ##
==========================================
+ Coverage 86.47% 86.59% +0.12%
==========================================
Files 42 42
Lines 1671 1701 +30
Branches 177 179 +2
==========================================
+ Hits 1445 1473 +28
- Misses 187 189 +2
Partials 39 39 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| /// <param name="builder">The <see cref="OpenFeatureBuilder"/> instance.</param> | ||
| /// <param name="implementationFactory"></param> | ||
| /// <returns></returns> | ||
| public static OpenFeatureBuilder AddHook<THook>(this OpenFeatureBuilder builder, Func<IServiceProvider, THook> implementationFactory) |
There was a problem hiding this comment.
I wonder if we should make implementationFactory optional. Then we can add a new hook using Services.AddHook<NoOpHook>(). In cases where the dependencies are already declared in the container, we should make use of it and not create a new ones.
| public static OpenFeatureBuilder AddHook<THook>(this OpenFeatureBuilder builder, Func<IServiceProvider, THook> implementationFactory) | ||
| where THook : Hook | ||
| { | ||
| var hookName = typeof(THook).Name; |
There was a problem hiding this comment.
They are being added as singleton. So if the hook already exists in the container should we really add them again? I would vote to do a continue and ignore adding more instances.
* This will allow consumers to easily add Hooks without forcing them to write a function everytime they do Signed-off-by: Kyle Julian <38759683+kylejuliandev@users.noreply.github.com>
This PR
Related Issues
Fixes #456
Notes
We inject any provided Hooks as Singletons in the DI container. We use keyed singletons and use the class name as the key. Maybe we'd want to use a different name to avoid conflicts?
I've done some manual testing with a sample weatherforecast ASP.NET Core web application
Follow-up Tasks
How to test