Remove automatic dependency injection for F# record fields, injecting…

[Arshia001]

… entire records instead

Injecting records looked like a good idea at first, since assigning names to parameters is a good thing. However, it causes a very big problem: What if we wanted to inject an entire record?

In F#, there are (AFAIK) two ways to create abstract sets of coherent operations:

• Use interfaces, and implement them with concrete types. Store data in fields.
• Use records containing functions. Implement operations by creating new instances of the record. Store data in closures.

Without going into detail about the pros and cons of each approach, I think we shouldn't force users of Saturn into using interfaces; and I can also think of a few other situations where injecting records could be useful.

Now, if we tried to inject this record:

type MyService = {
    AddInts: int -> int -> int
}

let myServiceFactory (logger: ILogger<MyService>) =
    {
        AddInts = fun x y ->
            logger.LogInformation("Adding two ints")
            x + y
    }

We'd get an error at runtime saying something like this: No registered service for FSharpFunc<int, int, int>. This is, of course, due to the dependency resolution code attempting to resolve each field of a record separately.

On the other hand, the argument for resolving record fields boils down to being able to name each dependency individually. After all, who'd want to write this?

get_di "/" (fun (d: IMyService * IMyOtherService * ILogger * IHttpClientFactory) ->
    let myService, myOtherService, logger, httpClientFactory = d
    ...

It's very highly unlikely that two functions would have the exact same dependencies. Record field resolution is best used with anonymous records:

get_di "/" (fun d: {| myService: IMyService; myOtherService: IMyOtherService; logger: ILogger; httpClientFactory: IHttpClientFactory |} -> ...)

However, this can more easily be accomplished using tuples like this:

get_di "/" (fun (myService: IMyService, myOtherService: IMyOtherService, logger: ILogger, httpClientFactory: IHttpClientFactory) -> ...)

There's also the fact that most people probably don't even expect fields of their record types to be resolved individually.

Given all of this, I feel the dependency resolution logic shouldn't attempt to resolve record fields. This change removes that logic and fixes the tests accordingly. I also modified the tuple tests to use individually named arguments, which looks much better than deconstruction.

[Arshia001]

@Krzysztof-Cieslak can I have your feedback on this please?

[Krzysztof-Cieslak]

I'm really sorry for the delay, there have been some changes in my professional life and I didn't have enough time to spend on Saturn in general.

So... I'm not sure if I'm a huge fan of this change. I tend to treat the record-of-functions approach as a kind of anti-pattern in F#. I'd rather "force" people to using interfaces than to support record-of-functions in a way that's proposed here, especially given that using interfaces is more "idiomatic" from ASP .NET point of view.

However, I'm happy to listen to more opinions here. CC: @isaacabraham @NinoFloris @baronfel

[Arshia001]

Without this change, Saturn would essentially be forcing people to only inject interfaces. I'm not quite sure that's a good thing.

Aside from the record of functions thing, one might also store some (immutable, unchangeable, constant) data in a record and want to inject that.

I'll also insist again that, from the user's point of view, this is a very unexpected thing to do.

[Arshia001]

Any updates?

[tforkmann]

@Arshia001 @Krzysztof-Cieslak asked us to
help out with the Saturn project, because of his professional life change. I’m trying to help out a bit but I’m just starting to dig deeper.
I personally like the current implementation more.

@cartermp Do you have an opinion on that?

[Arshia001]

Glad to know the Saturn project is going to be better supported :)

[cartermp]

My feeling is that this should probably be a discussion: https://github.com/SaturnFramework/Saturn/discussions

I don't think I have enough context to decide on this one way or another, and maybe laying out some scenarios in a discussion will help with that.

[Arshia001]

Great idea! Here: #339