Replace TaskBuilder with Ply

[NinoFloris]

It's a namespace change away.

This is the minimal diff, if we want we can go use some of the more specialized builders where it makes sense, this change is also not adding context insensitivity for reasons I expand on below.

I've been thinking of adding insensitive builders to Ply either way, but real use for it in an aspnetcore app is slim.

Aspnetcore doesn't seem to use ConfigureAwait(false) beyond app/server startup and servers:
https://github.com/dotnet/aspnetcore/search?p=1&q=configureawait&unscoped_q=configureawait
And this makes sense, the server is running without a context and therefore runs your middleware pipeline without context. Having a sync context in your middleware pipeline at all would be a serious bug so we could decide to drop the use of Insensitive entirely.

Unrelated, I do think I would like to add auto opening Builders in Ply for default/insensitive namespaces.

Final namespace changes for Giraffe could then be

- open FSharp.Control.Tasks.V2.ContextInsensitive 
+ open FSharp.Control.Tasks.ContextInsensitive

or

- open FSharp.Control.Tasks.V2.ContextInsensitive 
+ open FSharp.Control.Tasks

For users I would always recommend to do open FSharp.Control.Tasks

Why not this change:

• Moving to Ply comes at the cost of asking users to change their namespaces or explicitly add TaskBuilder.fs as a package reference. For many this work may come with seemingly zero upsides, which could make people seriously annoyed. Depending on the amount of breaks we're going to do in Giraffe 5.0 this could just be a minor thing though.

Why this change:

• There are some bugs in TaskBuilder for dealing with ValueTask, these become more painful now more apis are returning ValueTasks. (Code is not sufficiently generic rspeele/TaskBuilder.fs#31). Perf benefit of them is entirely negated too as they fall into the generic task-like path, allocs everywhere.
• There are some bugs around exception stacktraces that all CEs get for free from the compiler which Ply works around CE try catch with exception filters should use EDI instead of throw ex dotnet/fsharp#8529, hopefully my PR to fix this will go in for F# 5 after which this would be a moot point.
• Ply stacktraces are quite a bit nicer, less lambda noise and less 'infrastructure' frames overall.
• Ply is faster by default and it includes special builders for low alloc use under the Unsafe namespace, these bring F# very close to C# in terms of Task and ValueTask performance, allowing zero alloc use of ValueTasks.
• Taskbuilder netstandard2.0 support did happen a while back but releases are quite slow (we still love you @rspeele) and because of binary compatibility because of inlined implementation details they can be quite dangerous. Ply mitigates this by keeping all implementation details behind non inlined functions, this allows it to evolve quite a bit better.

Other things to do:

• Docs for Ply
• Should we do a 1.0.0 release for Ply?
• Benchmark of Giraffe with Ply to see improvement.
• Pray F# 6 contains a (value)task CE in FSharp.Core 😬

[dustinmoris]

Unrelated, I do think I would like to add auto opening Builders in Ply for default/insensitive namespaces.

👍

This looks good to me.

Depending on the amount of breaks we're going to do in Giraffe 5.0 this could just be a minor thing though.

This is my thinking.

What do others think? @TheAngryByrd @slang25 @Krzysztof-Cieslak @gerardtoconnor @isaacabraham @JonCanning (Including a few people who I know are using Giraffe a lot in one way or another)

[Krzysztof-Cieslak]

I'm 👍 for this change - given the number of changes in 5.0 this doesn't seem like something too annoying. I can't really comment on technical side of things - Nino definitely knows more about this side of things than me, so I assume all is good ;-)

[slang25]

I'm in favor too, the change isn't very painful, and we get all the benefits @NinoFloris has mentioned.

I was originally hoping we could hold off for the F# 5 support, but that's looking less likely, and this way we aren't tied to language version.

[isaacabraham]

No problem from my side - perf and more rapid releases is always good. I would think we'll miss the cut for SAFE 2.0 but we can move over after that.

[TheAngryByrd]

👍 as well. Namespace changes after a major release isn't a deal breaker. As long as it's documented/easily discoverable.

[slang25]

@NinoFloris Is there anything you want to do in Ply to get it to 1.0?

[pkese]

@NinoFloris Is there anything you want to do in Ply to get it to 1.0?

I second that.

If Ply is regarded as stable enough to put it into Giraffe, maybe it would be a good idea to then release Ply as 1.0 rather than 0.1.x

[Krzysztof-Cieslak]

Given that Giraffe 5.0 is currently in alpha stage I'd be for merging this as is, but for full 5.0 release, I'd want Ply 1.0.

[slang25]

Looks good, I would hope for a v1 for our next release as @Krzysztof-Cieslak says

The base branch was changed.

[dustinmoris]

I'm going to be difficult :)

Looks good, I would hope for a v1 for our next release as @Krzysztof-Cieslak says

If the consensus is that Ply should be v1 in order to be part of the next 5.0.0 Giraffe release then I'd suggest to hold back the merge of this PR until this PR has a v1 version of Ply in it, otherwise we'll end up with a Giraffe release sitting there with a dependency to wait for, possibly blocking the release when everything else feels ready otherwise. I 100% support this PR, just want to do it at the right time!

[NinoFloris]

Fyi Npgsql.FSharp and FsToolkit.ErrorHandling now ship with Ply 0.3.1 without issue, we may want to make sure Giraffe 5.0 moves as well before marking 5.0 stable. Thoughts?

[baronfel]

chants "do it, do it" from the sidelines

[Krzysztof-Cieslak]

I'd be really happy to move to Ply, but as far as I remember one thing we wanted is doing Ply 1.0 release before we include it in Giraffe.

[dustinmoris]

What is missing in Ply 0.3.1 that we are waiting for 1.0.0?

[Krzysztof-Cieslak]

@dustinmoris As far as I remember, I don't think there were any technical reasons, but rather you wanted to have "stable" dependencies?

[dustinmoris]

Ah ok, I think someone suggested to me (I think it was maybe on a call) that 0.x.x suggests that it's still in pre-release state and therefore I agreed to wait until it's stable, but it seems that Ply is already stable and used by other projects and since @NinoFloris himself is recommending it for general use I'd be happy to merge it now and release with the very next release, which I'm aiming for this weekend.