Skip to content
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

Consider making ICarterModule.AddRoutes a static abstract method #338

Open
mariusz96 opened this issue Mar 16, 2024 · 5 comments
Open

Consider making ICarterModule.AddRoutes a static abstract method #338

mariusz96 opened this issue Mar 16, 2024 · 5 comments

Comments

@mariusz96
Copy link

mariusz96 commented Mar 16, 2024

Hi,

Being accustomed to controllers I installed Carter and ran into an issue similar to these when injecting a scoped service into a module's constructor threw an exception:

And I also think that this unknowingly affects many people who inject transient services (ie MediatR library) into a module's constructor.

It appears to me that with an interface like this:

public interface ICarterModuleV2 // Naming things is hard.
{
    static abstract void AddRoutes(IEndpointRouteBuilder app);
}

this could be completely avoided and AddRoutes could still be called through reflection (easy, less efficient) or a source generator (hard, as efficient as hard-coded by a dev).

What do you think? Would this be a worthwhile addition?

EDIT: Turns out there's even a tutorial for this exact source generator: https://dev.to/joaofbantunes/mapping-aspnet-core-minimal-api-endpoints-with-c-source-generators-3faj.

@jchannon
Copy link
Member

As #279 says, you shouldn't be injecting services into your classes constructors, minimal API does not work like that. They should be in the RequestDelegate

public class MyModule : ICarterModule
{
    public void AddRoutes(IEndpointRouteBuilder app)
    {

        app.MapGet("/home", IService service => {
          
          var result = service.Foo();
          
          return Results.Ok(result);
        });
   }
}

@mariusz96
Copy link
Author

mariusz96 commented May 18, 2024

As #279 says, you shuldn't be injecting services into your cla*** constructors, minimal API does not work like that. They sh*uld be in the RequestDelegate

public cla** MyModule : ICarterModule
{
    public void AddRoutes(IEndpointRouteBuilder app)
    {

        app.MapGet("/h*me", IService service => {
          
          var result = service.Foo();
          
          return Results.Ok(result);
        });
   }
}

Yes, I understand that. But with a static abstract method: static abstract void AddRoutes(IEndpointRouteBuilder app), using a dependency injected into a constructor would not even compile. So it would prevent me from making an error and enforce correct usage.

Which is good for a library interface, no?

@jchannon
Copy link
Member

jchannon commented May 18, 2024 via email

@jchannon
Copy link
Member

I've had a play and making the interface have the abstract static method would be an aid to help this.

As it's a breaking change I will do this for .NET9. For now I will add some XML docs as a pointer not to inject dependencies into classes.

What it will look like:

/// <summary>
/// An interface to define HTTP routes
/// </summary>
/// <remarks>Implementations of <see cref="ICarterModule"/> should not inject constructor dependencies. All dependencies should be supplied in the route <see cref="RequestDelegate"/></remarks>
public interface ICarterModule
{
    /// <summary>
    /// Invoked at startup to add routes to the HTTP pipeline
    /// </summary>
    /// <remarks>Implementations of <see cref="ICarterModule"/> should not inject constructor dependencies. All dependencies should be supplied in the route <see cref="RequestDelegate"/></remarks>
    /// <param name="app">An instance of <see cref="IEndpointRouteBuilder"/></param>
    static abstract void AddRoutes(IEndpointRouteBuilder app);
}

@jchannon
Copy link
Member

@mariusz96 can you test the latest Carter version, it has an analyzer that warns if you have constructor dependencies 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants