[Feature] Introduce a task-based API and remove Result<'T, 'E> API friction
Zaid-Ajaj opened this issue · 16 comments
This is an attempt to tackle #64 right now, the functions Sql.execute, Sql.executeRow, Sql.executeNonQuery etc. return data in the form of Async<Result<'SomeType, exn>>
This is fine in many cases but it can cause friction when:
- You don't want
Resultand instead choose totry ... catchthe SQL code only on rare occasions - You are working with
task(for example in Saturn/Giraffe) and don't want the syntax and performance overhead of converting betweenasyncandtask
I propose to fix the problems above by introducing mutually exclusive namespaces within this same nuget package that offers different return types BUT with the exact API of the Sql functions:
Npgsql.FSharpstays the same and continue to returnAsync<Result<'T, exn>>(package stays backward-compatible without breaking changes)Npgsql.FSharp.NoResultexposes API functions that returnAsync<'T>Npgsql.FSharp.Tasksexposes API functions that returnTask<'T>with the help of Ply
Yes, the downside is of course the duplication of code in each namespace but I think it is worth the effort of removing as much overhead as possible and of course we can extract the common parts in a module (the RowReader for example).
What do you think @aspnetde @TheAngryByrd @baronfel @NinoFloris
Is there a reason you don't want Sql.execute, Sql.executeAsync, Sql.executeTask, Sql.executeExn/Sql.executeUnsafe(?) in the same module. Sure it's a bigger API surface, but the names and types are obvious.
Or you could still explicitly namespace them (eg. Npgsql.FSharp.Tasks.Sql.execute which returns a Task<Result<'T, exn>>), but also present them via. a top-level module that uses the explicit name (eg. Sql.executeTask) but it might make namespace ordering a bit awkward.
Just as an aside, the likes of https://github.com/demystifyfp/FsToolkit.ErrorHandling has a separate package explicitly for TaskResult as it's an optional/opinionated choice, but obviously adds a bit more complication with version management for maintainer(s).
I now had to work around Result in my Giraffe application. Which has some ugly side effects in case an exception is thrown, e.g., I need to manually parse the inner exception to get some meaningful debug information.
So I absolutely welcome this initiative! If you introduce multiple namespaces or just add additional functions to the existing namespaces is a matter of taste. Maybe following @absolutejam's suggestion would provide better long-term maintainability, as those functions would live side-by-side and not in different files/modules.
Is there a reason you don't want Sql.execute, Sql.executeAsync, Sql.executeTask, Sql.executeExn/Sql.executeUnsafe(?) in the same module. Sure it's a bigger API surface, but the names and types are obvious.
@absolutejam It is purely subjective on my end, there just aren't good names for these variants IMO! I think if you know which namespace you need, that you will forget about it in a minute or two to get going. Keeping the same functions also means that the analyzer will support the different namespaces without any code changes there so that is a big plus for me (and for the people who are actually using it) 😄
I have checked out FsToolkit.ErrorHandling and I think it overcomplicates things a bit, that's why I suggested to start off with one package, open the namespace you want and get things done. I am thinking this proposition has the least mental overhead but just like my initial API proposition, I might be wrong on that one too (which is I opened an issue to discuss)
@aspnetde Thanks for the feedback! After looking at lots of Saturn code, I realized how much better the API would be when all the friction is removed 💯
Another reason why I opened this issue is hoping that someone would pick it up 😁 I would really really appreciate it, especially since it is a lot of copy-paste and writing tests for the different variants. Please let me know if you want to take a stab at it 🙏 🙇
Hehe. Yeah, I absolutely would like to look into it – if I wasn't running at 120% load at the moment. So I could give it a try, but I cannot say when that would be :-/.
Yeah no worries, I am in the same boat. I will just leave it here for now for a while, it is not an urgent fix anyways 😄
Npgsql.FSharp.Tasks exposes API functions that return Task<'T> with the help of Ply
Since it sounds pretty likely that F# vNext is going to get native Task/ValueTask support, does that change anything public facing or just the implementation (could potentially not use Ply when running under that version) ?
What I see as the biggest - theoretical - downside to reusing the same fully qualified module name across packages is once some downstream package takes a dependency on one of the options you can't use another one in your app without running into type name ambiguities.
F# doesn't have any extern alias support like C# to work around that fsharp/fslang-suggestions#513
https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/extern-alias
I also like the idea with different namespaces, already seen it in other projects like SchlenkR.FsHttp as an example.
Maybe you should add some labels to the issue like "Easy" or "YourFirstPR" to attract beginner contributors :)
By the way. NpgSql has a new major version. It still works, but I've seen some MissingMethod exceptions if not pinned any versions.
Since it sounds pretty likely that F# vNext is going to get native Task/ValueTask support, does that change anything public facing or just the implementation (could potentially not use Ply when running under that version) ?
@ericsampson It doesn't change the public facing API, only the implementation and we can all migrate after F# supports tasks natively.
What I see as the biggest - theoretical - downside to reusing the same fully qualified module name across packages is once some downstream package takes a dependency on one of the options you can't use another one in your app without running into type name ambiguities.
@NinoFloris I don't I'll encounter this issue because the idea is to keep one package and different namespaces
By the way. NpgSql has a new major version. It still works, but I've seen some MissingMethod exceptions if not pinned any versions.
@vilinski Thanks for letting me know, then we can also bump up Npgsql dependency version with the feature 😄
Ah I read mutually exclusive namespaces too quickly ;)
Just as an aside, the likes of https://github.com/demystifyfp/FsToolkit.ErrorHandling has a separate package explicitly for TaskResult as it's an optional/opinionated choice.
The main reason it's a different package entirely is because we support Fable and having ifdefs everywhere in the code is real messy, especially with regards to tooling, or at least when I made that package years ago.
As of v3.12 the library has implemented the namespace Npgql.FSharp.Tasks which offers the exact same API but with tasks and without Result, I haven't gotten around to fully test it but in principle it should work 🤞 can you please give it a try and let me know how it goes? I have also updated the README docs to reflect the changes
@vilinski Latest has now updated Npgsql to v5.0 and all existing tests worked without changes 😄
That's great news! I will give it a try as soon as possible.
@Zaid-Ajaj tried out, refactored my small program to Tasks, works great!
That's great news! I will give it a try as soon as possible.
Today I finally found the two hours of time to make the switch to Npgsql.FSharp.Tasks. Can't say anything about performance, but it compiles, tests succeed, and overall it is looking good. Great job!