SaturnFramework / Saturn

Opinionated, web development framework for F# which implements the server-side, functional MVC pattern
https://saturnframework.org
MIT License
715 stars 109 forks source link

Initial implementation of automatic dependency injection #276

Closed Arshia001 closed 3 years ago

Arshia001 commented 3 years ago

I was going to collect feedback on this matter in #275, but the implementation turned out to be straight-forward enough that I decided to submit an implementation as a basis for discussion around the issue.

Basically, for every CE builder method that accepts a function or the result of another CE, a new method of the same name + _di is added. These new query operators accept functions with one more argument, which is assumed to contain service references and will be resolved at runtime via HttpContext.RequestServices.

To give an example, this code is now allowed:

let someRouter (logger: ILogger<MyType>, repo: IRepository<MyEntity>) = router {
    get "/" (fun _ ctx ->
        logger.LogInformation("Will return entity details")
        ctx.WriteTextAsync (repo.GetEntityDetails ())
    )
}

let appRouter = {
    forward_di "/entity" someRouter
}

Note the query operator is forward_di. This allows code to remain explicit, and avoids a few cases where an overloaded operator could confuse the compiler's type resolver.

Automatic dependency resolution is supported on controllers, controller actions (was already available before), pipelines, routers, channels, and everything in between.

This implementation is in no way complete. It's meant to serve as a suggestion and basis for further refinement before it is ready. Specifically, at least the following points need to be addressed:

I'd love to get some feedback on this matter.

Arshia001 commented 3 years ago

@Krzysztof-Cieslak is this project unmaintained by any chance?

Krzysztof-Cieslak commented 3 years ago

Yes, the project is maintained šŸ™„

Arshia001 commented 3 years ago

That's great! šŸ˜

Can I have your feedback on this pr then?

Krzysztof-Cieslak commented 3 years ago

Two quick comments/questions:

  1. Why do we need to have additional _di functions in the controller CE? The dependency injection like that was already working in the controller CE, as far as I understand.
  2. In general, I'm not a fan of having all those _di custom operations- we should add them as overloads for existing custom operations rather than separate custom operations

Otherwise, it looks really good, I'll be happy to have this.

Arshia001 commented 3 years ago
  1. Purely for consistency/symmetry with the rest of the functions.
  2. I did try to use overloads, but I ran into a bunch of type inference problems. I'll try to get it working with overloads again.
Arshia001 commented 3 years ago

@Krzysztof-Cieslak

When I try to use overloaded methods, two errors happen.

First, for get, post, pipe_through, etc., automatic type resolution on next and ctx fails with Lookup on object of indeterminate type based on information prior to this program point. A type annotation may be needed prior to this program point to constrain the type of the object. This may allow the lookup to be resolved. I suppose this is because the compiler doesn't know which overload to use, even though it looks obvious to me (maybe it has something to with F#'s single pass compilation?) This requires users to annotate their method arguments with types, which both breaks existing code and IMHO is more of a burden than the additional _di in the query operator name.

Second, getf, postf and friends fail to resolve completely with

No overloads match for method 'GetF'.

Known types of arguments: RouterState * string * (string -> HttpFunc -> 'a -> HttpFuncResult) when 'a :> AspNetCore.Http.HttpContext

Available overloads:
 - member RouterBuilder.GetF : state:RouterState * path:PrintfFormat<'a0,'a1,'a2,'a3,'f> * action:('a5 -> 'f -> HttpHandler) -> RouterState // Argument 'path' doesn't match
 - member RouterBuilder.GetF : state:RouterState * path:PrintfFormat<'a0,'a1,'a2,'a3,'f> * action:('f -> HttpHandler) -> RouterState // Argument 'path' doesn't match

I don't know why this happens, since the path argument clearly is a PrintfFormat, but it happens anyway, and I don't know how it can be worked around either. This also breaks existing code.

Last, I think the reason why the existing overloads in Controller work is because there, the dependency argument is supplied in the last argument. This is not an option for other query operators because they need to work with the HttpHandlers created by the workflows, e.g.

let myRouter (logger: ILogger<SomeType>) = router { ... } // Can't move the dependency argument around!

All in all, the extra _di looks like a much better option to me. If you don't think it's a good option, the errors above have to be resolved in a way that doesn't break existing code, but I don't know how.

I can push the code if you want to take a look. Bear in mind though, the tests won't compile.

Krzysztof-Cieslak commented 3 years ago

Yeah, please push the code, custom operations are sometimes tricky, I'll take a look.

Arshia001 commented 3 years ago

Done. My fingers are crossed.

Arshia001 commented 3 years ago

Any luck yet, @Krzysztof-Cieslak?

Krzysztof-Cieslak commented 3 years ago

Thanks a lot for working on this, and sorry it took me so long. ā¤