event-driven-dotnet / EventDriven.EventBus.Dapr

Event bus abstraction over Dapr pub/sub
MIT License
22 stars 5 forks source link

Suggestions around routing #2

Closed tonysneed closed 3 years ago

tonysneed commented 3 years ago

From @rynowak:

Here's some feedback based on my experiences supporting ASP.NET Core and Dapr for .NET. These are just my opinions so feel free to ignore my ideas if they are a bit fit for what you're trying to accomplish.

Hiding routing

I don't recommend that libraries hide routing like you're doing here.

This topic is common enough that I added it to the official docs.

DO build on top of IEndpointRouteBuilder. This allows users to compose your framework with other ASP.NET Core features without confusion. Every ASP.NET Core template includes routing. Assume routing is present and familiar for users.

The main reason why I don't recommend what you are doing is that it doesn't compose. What if the user needs to add the authorization middleware? What if they are using Blazor server? We actually had this bug in actors.

Creating a psuedo-middleware that wraps a bunch of other middleware at first seems like the simple option, but it has problems when the user needs to control the pipeline.

I think you could reduce your standard code sample to something like:

app.UseRouting();

app.UseEndpoints(endpoints =>
{
    endpoints.MapSubscribeHandler();
    endpoints.MapEventBus();
});

This looks a lot like the code a Dapr user is already going to have in their project and you've only added one line. This has the benefit of exposing the truth of the mechanics to the user in a way that they can customize (event bus IS a routing feature).

EndpointConventionBuilder

If you do take this advice, I'd recommend also that MapEventBus returns a convention builder (see example) This lets users apply metadata like authorization or cors or other diagnostics to their endpoints.