Avanade / Liquid-Application-Framework

Liquid Application Framework documentation, useful links and sample project
MIT License
491 stars 57 forks source link

Remove the need for creating the mandatory constructor overload #49

Closed guilhermeluizsp closed 3 years ago

guilhermeluizsp commented 3 years ago

During the presentation stream on YouTube, one of the team members was introducing a sample made with Liquid. At some point (around 59m15s), he also needed to demonstrate how things are done at the HTTP layer of the application, which uses ASP.NET Core. The first thing he stumbles upon is the need of some very specific constructor parameters for a very specific constructor overload — a requirement enforced by the language and the runtime for inheritance to work yadda yadda yadda. He does what probably anyone would do at presentations/teaching lessons: just kind of skips it (aka "don't worry about it for now") because it's not relevant for the context. The aforementioned expression "don't worry about it for now" is being taken seriously by Microsoft's C# and ASP.NET Core engineering team. They acknowledged it as a problem and are actively working on solutions to improve these sort of initial barriers for newcomers, such as the removal of the main method. Lately, we can also see David Fowler (an Architect of the ASP.NET Core team) working on similar efforts to ease things out for the framework.

That being said, I think we're facing the same issue here: there's an overwhelming amount of information for beginners to create Controllers.

What if we take the same path as the Microsoft engineering team? Let's make things easier by removing this cognitive complexity-like thing from the code. My proposal is to make this:

using Liquid.Core.Context;
using Liquid.Core.Localization;
using Liquid.Core.Telemetry;
using Liquid.WebApi.Http.Controllers;
using MyProject.Domain.Queries;
using Microsoft.AspNetCore.Mvc;
using System.Threading.Tasks;

[ApiController]
[Route("/api/[controller]")]
public class MyController : BaseController
{
    public MyController(ILoggerFactory loggerFactory,
                        IMediator mediator,
                        ILightContext context,
                        ILightTelemetry telemetry,
                        ILocalization localization)
        : base(loggerFactory,
                  mediator,
                  context,
                  telemetry,
                  localization)
        { }

    [HttpGet()]
    public async Task<IActionResult> SearchMovies([FromQuery(Name="nameSearch")] string nameSearch) =>
        await ExecuteAsync(new ListMoviesQuery() { SearchString = nameSearch });
}

turn into this:

using Liquid.WebApi.Http.Controllers;
using MyProject.Domain.Queries;
using Microsoft.AspNetCore.Mvc;
using System.Threading.Tasks;

[ApiController]
[Route("/api/[controller]")]
public class MyController : BaseController
{
    [HttpGet()]
    public async Task<IActionResult> SearchMovies([FromQuery(Name="nameSearch")] string nameSearch) =>
        await ExecuteAsync(new ListMoviesQuery() { SearchString = nameSearch });
}

ASP.NET Core is an extraordinary well built and modular framework, and this can be easily achieved by either decorating its ControllerFactory/ControllerActivator or simply by annotating the BaseController properties with the [FromServices] attribute.

The tradeoff here is that some things would become implicit (for good) and the properties from the BaseController class would need to have public setters, as they are going to be filled dynamically. We're also trading compile time for run-time, but, of course, people wouldn't need to worry about creating constructors, so less of "why do we need to put this thing here?".

lucianareginalino commented 3 years ago

In the first place, thanks for the issue! I think that a good solution is turn only IServiceProvider explicit in the Controller's constructor, because de Attibute [FromServices] is only valid on 'parameter' declarations.

lucianareginalino commented 3 years ago

To solve this, in an internal discussion by the team of maintainers of the framework, we understand that the solution should be make only the IServiceProvider explicit in the builder, and obtain the services to populate the properties, keeping them private. In this way we will keep the developer clear that there are dependencies, and facilitate the injection manipulation, to enable, for example, test mocks. In addition, we understand a good solution because in a future evolution of the product that needs a new dependency, this will be possible without create a breaking change, or worrying about backwards compatibility.