-
-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduced DI #25
base: master
Are you sure you want to change the base?
Introduced DI #25
Conversation
…ber) at project level
@@ -0,0 +1,49 @@ | |||
<Project Sdk="Microsoft.NET.Sdk"> | |||
|
|||
<PropertyGroup> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FIX: Let's merge it into the regular project. I'm fine with having hosting.abstractions and Polly dependency in the main project. It's much more accessible when user installs just a package and have all available (of course, unless that's enforcing too much from the peer dependency side).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've no strong opinion against it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lsfera, could you change that in the PR?
src/SubscriberWorker/Program.cs
Outdated
var builder = Host.CreateApplicationBuilder(args); | ||
|
||
builder.Services | ||
.AddBlumchen<SubscriberWorker<UserCreatedContract>, UserCreatedContract>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CONCERN: Does that mean that we need to call AddBlumchen for Contracts separately? My preference would be to have a single AddBlumchen with additional lambda as param for configuring dependencies. See example how we did it in Marten: https://martendb.io/configuration/hostbuilder.html#bootstrapping-marten.
It should also register all needed default configurations allowing user to override that later on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lsfera see the comments, but that's a nice first step to have it.
@@ -74,10 +73,9 @@ public SubscriptionOptionsBuilder WithReplicationOptions(ReplicationSlotManageme | |||
} | |||
|
|||
[UsedImplicitly] | |||
public SubscriptionOptionsBuilder Consumes<T, TU>(TU consumer) where T : class | |||
where TU : class, IConsumes<T> | |||
public SubscriptionOptionsBuilder Consumes<T>(IHandler<T> handler) where T : class |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
COMMENT: In the long term I'd also like to allow handlers that don't use marker interfaces, but that's something to tackle in the dedicated PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lsfera, thank you for including my suggestions and the hard work. I'm fine with pulling that in after merging the dependency injection project with the regular one. 👍
Superseeded by #28!
Exposed AddBlumchen() capability.
Verify demo project SubscriberWorker for usage
closes #15