Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TaskResult to Ply #97

Merged
merged 2 commits into from
Aug 29, 2020
Merged

Conversation

NinoFloris
Copy link
Contributor

Port of TaskResult to Ply

  • Moves all CEs from Taskbuilder to Ply
  • Inlines a lot more delegation methods, better for perf and stack traces.
  • Implements loops on top of uply which is as cheap as it can get.
  • Removes the net461 TFM as Ply does not support it.
  • Pins FSharp.Core to .Net Core 2.1 LTS implicit reference version.

There's one unresolved matter, context insenstive binds. Ply does not support them out of the box (never had a need for them on .net core) so I have replaced them with normal binds to get this compiling. We could leave it, implement them here, or I could push a new version of Ply that has them; I don't have any strong preference.

@FoggyFinder
Copy link
Contributor

Just curious: Why? What advantages Ply gives over TaskBuilder ?

@TheAngryByrd
Copy link
Collaborator

Thanks a ton for this @NinoFloris!

Inlines a lot more delegation methods, better for perf and stack traces.

❤️

Implements loops on top of uply which is as cheap as it can get.

❤️

Removes the net461 TFM as Ply does not support it

This one might really hurt. This has been the main reason I haven't ported to Ply yet. Someday I'll want to give up netfx, but I'm unsure if I'd want to do that today.

There's one unresolved matter, context insenstive binds. Ply does not support them out of the box (never had a need for them on .net core) so I have replaced them with normal binds to get this compiling

Yeah I could this this affecting people as well, most likely anyone dealing with UI work. I'm unaware of anyone using it like this currently, but right after releasing, I know I'll get a bunch of issues about it 😅 .


Given those breaking changes, I don't think I can accept it as is. However, I'd be open to making a separate project/package (FsToolkit.ErrorHandling.PlyResult or something) that targets just Ply (Big old copy paste of what you have). It allows people to try it out without breaking their current projects today in non obvious ways. I'll make it heavily known in the release notes that we're planning on moving to Ply so please try it out, but in reality I know a subset won't and it'll be a surprise to them.

@NinoFloris
Copy link
Contributor Author

Yeah I could this this affecting people as well, most likely anyone dealing with UI work. I'm unaware of anyone using it like this currently, but right after releasing, I know I'll get a bunch of issues about it 😅 .

Same, considering we want to get a bunch of libraries on board with Ply (which really just need ConfigureAwait(false)) I'll integrate it into Ply sometime soon.

However, I'd be open to making a separate project/package (FsToolkit.ErrorHandling.PlyResult or something) that targets just Ply (Big old copy paste of what you have)

I was thinking the same as a backup so I'm happy to hear you suggest it :)
Initially I had a line in my opening comment about releasing and how it would at the very least require a major version bump, that route could also be an option if I get the context insensitive things sorted.

Generally speaking it's as described in the Giraffe issue, a change of namespaces, one bulk search and replace should do it.

@NinoFloris
Copy link
Contributor Author

NinoFloris commented Aug 17, 2020

Ah and on the netfx tfm, ns2.0 should be properly supported under net472 right?

Ply deps are like this
Screenshot 2020-08-17 at 17 02 48

@baronfel
Copy link

Yes, net472 should be totally compatible with ns2.0 (per the infamous compatibility chart).

I would honestly be very supportive of dropping the net461 TFM. We're about to do so in FSharp.Compiler.Services, for example, and request that folks use net472 at minimum, so we can ship a netstandard-only dll. It's just so much less complexity, and net472 is positively ancient at this point (more than 2 years old)!

@TheAngryByrd
Copy link
Collaborator

Ah and on the netfx tfm, ns2.0 should be properly supported under net472 right?

Yeah I think this is true.

It's just so much less complexity, and net472 is positively ancient at this point (more than 2 years old)!

I think we forget the pace as which Windows developers projects tend to move 😢 . 2 years is pretty new on their timescale.

My personal opinion about a library like this is, it's should be considered low level and should try to support as much as it can. I don't want to have to strand people if I don't have to, especially if it's not that much work on my part.

However, I'd definitely like @tamizhvendan's opinion on forcing net472 at a minimum for this library.

@NinoFloris
Copy link
Contributor Author

@TheAngryByrd the tasks the FsToolkit builders work on top of are once again ConfigureAwait(false) augmented!

(the downside is that FsToolkit CEs are, and always were, quite broken for UI work)

@baronfel
Copy link

@FoggyFinder to answer your question, ply is

  • More efficient
  • Can handle a wider variety of task-like awaitables
  • Handles a few correctness gotcha around execution context bubbles

And you can check the readme for ply where @NinoFloris has gone into much deeper detail for specifics about those point and more!

@NinoFloris
Copy link
Contributor Author

Pushed a tiny fix to ediPly (edge case scenario), the "stacktraces like C#" refactor brought some unfortunate churn...

@NinoFloris
Copy link
Contributor Author

Any remaining work for me to take care of?

@TheAngryByrd
Copy link
Collaborator

Any remaining work for me to take care of?

I think it looks good on your end, I just would like @tamizhvendan's opinion on dropping net461 since I'm still somewhat hesitant.

@TheAngryByrd
Copy link
Collaborator

@NinoFloris I just fixed Build Checks in #98, would you might giving this a rebase? Thanks 😄

@NinoFloris
Copy link
Contributor Author

Done!

@TheAngryByrd TheAngryByrd merged commit 491c4e5 into demystifyfp:master Aug 29, 2020
TheAngryByrd added a commit that referenced this pull request Aug 29, 2020
- Switches TaskResult Library from TaskBuilder to Ply. Credits [Nino FLoris](https://github.com/NinoFloris) - (#97)
- This change replaces [TaskBuilder](https://github.com/rspeele/TaskBuilder.fs) with [Ply](https://github.com/crowded/ply).  Ply has better performance characteristics and more in line with how C# handles Task execution.  To convert from TaskBuilder to Ply, replace the namespace of `FSharp.Control.Tasks.V2.ContextInsensitive` with `FSharp.Control.Tasks.NonAffine`.
- This also removes the TargetFramework net461 as a build target. Current netstandard2.0 supports net472 fully according to [this chart](https://docs.microsoft.com/en-us/dotnet/standard/net-standard#net-implementation-support). It's recommended to upgrade your application to net472 if possible. If not, older versions of this library, such as 1.4.3, aren't going anywhere and can still be consumed from older TargetFrameworks.
TheAngryByrd added a commit that referenced this pull request Nov 18, 2020
- Switches TaskResult Library from TaskBuilder to Ply. Credits [Nino Floris](https://github.com/NinoFloris) - (#97)
- This change replaces [TaskBuilder](https://github.com/rspeele/TaskBuilder.fs) with [Ply](https://github.com/crowded/ply).  Ply has better performance characteristics and more in line with how C# handles Task execution.  To convert from TaskBuilder to Ply, replace the namespace of `FSharp.Control.Tasks.V2.ContextInsensitive` with `FSharp.Control.Tasks.NonAffine`. -
- This also removes the TargetFramework net461 as a build target. Current netstandard2.0 supports net472 fully according to [this chart](https://docs.microsoft.com/en-us/dotnet/standard/net-standard#net-implementation-support). It's recommended to upgrade your application to net472 if possible. If not, older versions of this library, such as 1.4.3, aren't going anywhere and can still be consumed from older TargetFrameworks.
- Switch to use Affine for Task related. Credits [@Swoorup](https://github.com/Swoorup). - (#107)
TheAngryByrd added a commit that referenced this pull request Nov 20, 2020
- Switches TaskResult Library from TaskBuilder to Ply. Credits [Nino Floris](https://github.com/NinoFloris) - (#97)
- This change replaces [TaskBuilder](https://github.com/rspeele/TaskBuilder.fs) with [Ply](https://github.com/crowded/ply).  Ply has better performance characteristics and more in line with how C# handles Task execution.  To convert from TaskBuilder to Ply, replace the namespace of `FSharp.Control.Tasks.V2.ContextInsensitive` with `FSharp.Control.Tasks.NonAffine`. -
- This also removes the TargetFramework net461 as a build target. Current netstandard2.0 supports net472 fully according to [this chart](https://docs.microsoft.com/en-us/dotnet/standard/net-standard#net-implementation-support). It's recommended to upgrade your application to net472 if possible. If not, older versions of this library, such as 1.4.3, aren't going anywhere and can still be consumed from older TargetFrameworks.
- Switch to use Affine for Task related. Credits [@Swoorup](https://github.com/Swoorup). - (#107)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants