Update Samples (Console, Netfx, Functions) #78
Conversation
| { | ||
| tasks.AddOrchestrator("HelloSequence", async context => | ||
| services.AddDurableTaskClient(builder => |
There was a problem hiding this comment.
Right now, client and worker must be registered independently. I am not against worker also bringing in the client, but I'd like to gather feedback on that before committing time to it.
There was a problem hiding this comment.
I feel like the common case would be registering both, and that registering one or the other would be the advanced case. Could we simply have an AddDurableTask extension method that registers both? Or is that complicated because we've put them in two separate packages with no top-level package to give developers everything?
There was a problem hiding this comment.
I agree the common case will probably be registering both for worker scenarios only. I think it will also be common to want to register only the client (e.g., Web API with client, but worker is in a separate console app). I think in the future we should explore AddDurableTaskWorker also registering the corresponding client, but yes, the package separation complicates it. I am open to adding a Worker --> Client package reference, but it will require some design around integrating the calls together. I think this can be left to post-GA.
| await context.CallActivityAsync<string>("SayHello", "Seattle"), | ||
| }; | ||
| builder.UseGrpc(); | ||
| builder.RegisterDirectly(); |
There was a problem hiding this comment.
This line registers the DurableTaskClient directly to the service collection. I am wondering if we should register the "Default" (no name provided) client to the service container?
You can always import IDurableTaskClientProvider and get a client by name,
There was a problem hiding this comment.
I'm afraid I don't understand the type architecture well enough to understand the question. I also worry customers might struggle to understand like I am. The call to host.Services.GetRequiredService<DurableTaskClient>() makes me a tiny bit uneasy since it's not particularly discoverable, but I think it's okay. I do worry about this builder.RegisterDirectly() requirement though. I would never think to call that. Could be make this the default configuration?
There was a problem hiding this comment.
#81 partially addresses this - removing the need for RegisterDirectly in this case. I will update this PR to reflect that.
There was a problem hiding this comment.
Are we able to remove this now?
jviau
force-pushed
the
update-samples
branch
from
500a1b7 to
b6052c4
Compare
| @@ -7,9 +7,12 @@ | |||
| </PropertyGroup> | |||
|
|
|||
| <ItemGroup> | |||
| <PackageReference Include="Microsoft.Extensions.Logging" Version="6.0.0" /> | |||
| <PackageReference Include="Microsoft.DurableTask.Client" Version="0.4.1-beta" /> | |||
There was a problem hiding this comment.
Ah, so this is why your PRs haven't needed to update the samples until now. :) I wonder if we can still keep these package references in as comments, maybe with placeholder version values. My intention was to make it as easy as possible for users to copy/paste the samples and have them work in their own projects, which won't have access to the project references.
There was a problem hiding this comment.
Sure - I will introduce the commented-out package references when we release the next NuGet package
| // You can configure custom data converter multiple ways. One is through worker/client options configuration. | ||
| // Alternatively, data converter will be used from the service provider if available (as a singleton) AND no | ||
| // converter was explicitly set on the options. | ||
| services.AddSingleton<DataConverter>(JsonDataConverter.Default); |
There was a problem hiding this comment.
I really like how you added these optional configuration calls and described them with the comments. 👍🏽
| await context.CallActivityAsync<string>("SayHello", "Seattle"), | ||
| }; | ||
| builder.UseGrpc(); | ||
| builder.RegisterDirectly(); |
There was a problem hiding this comment.
I'm afraid I don't understand the type architecture well enough to understand the question. I also worry customers might struggle to understand like I am. The call to host.Services.GetRequiredService<DurableTaskClient>() makes me a tiny bit uneasy since it's not particularly discoverable, but I think it's okay. I do worry about this builder.RegisterDirectly() requirement though. I would never think to call that. Could be make this the default configuration?
| { | ||
| tasks.AddOrchestrator("HelloSequence", async context => | ||
| services.AddDurableTaskClient(builder => |
There was a problem hiding this comment.
I feel like the common case would be registering both, and that registering one or the other would be the advanced case. Could we simply have an AddDurableTask extension method that registers both? Or is that complicated because we've put them in two separate packages with no top-level package to give developers everything?
# Conflicts: # samples/NetFxConsoleApp/Program.cs
jviau
changed the title
Updates the ConsoleApp and NetFxConsoleApp samples to the latest way of using the packages.