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

Implement async handling option: "async" | "task" for the fsharp target #31

Closed
Zaid-Ajaj opened this issue Mar 29, 2021 · 7 comments
Closed
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@Zaid-Ajaj
Copy link
Owner

Basically:

{
   ["asyncReturnType"]: < "async" | "task" >
}

Where async is the default. To make the generated functions either return Async<'T> or Task<'T>

For the latter you would need to include Ply as a dependency to the generated F# project.

Related to #29

Might need a better API for generating the projects and their packages.

@Numpsy
Copy link
Contributor

Numpsy commented Jun 14, 2021

Is Numpsy@d4f91f7 the right sort of thing?

Notes:

  • I'm largely making it up as I go along, and am not certain what stringBuffer is giving you in some cases.
  • Is Async.AwaitTask/Async.RunSynchronously the approach you'd want with tasks? (When I've done the sync-over-async thing with tasks in C# it was using the GetAwaiter().GetResult() type approaches, not sure what the best approach is in F#).

@Zaid-Ajaj
Copy link
Owner Author

Zaid-Ajaj commented Jun 14, 2021

@Numpsy you are on the right track, here is a checklist from the top of my head:

  • don't worry about stringBuffer, it just concatenates the strings
  • when checking if isNull parsedJson.["asyncReturnType"] || string parsedJson.["asyncReturnType"] = "async", the expression string parsedJson.["asyncReturnType"] should be lower-cased (same when validating)
  • should return a parse error if asyncReturnType = task && target = fable because Fable doesn't support tasks
  • should include nuget Ply @ v0.3.1 when asyncReturnType = task
  • should open namespace FSharp.Control.Tasks when asyncReturnType = task (namespace from Ply)
  • Using GetAwaiter().GetResult() for sync sounds good. It doesn't matter that much, both should work fine (if it is easier to leave it as is, I am fine with that)

@Numpsy
Copy link
Contributor

Numpsy commented Jun 14, 2021

* don't worry about stringBuffer, it just concatenates the strings

I'd thought it did, but then it looked like some of the existing uses only contain a single string, so I wasn't sure if I was missing something :-(

* when checking `if isNull parsedJson.["asyncReturnType"] || string parsedJson.["asyncReturnType"] = "async"`, the expression `string parsedJson.["asyncReturnType"]` should be lower-cased (same when validating)

Hmm, I copied that from the existing code @

if isNull parsedJson.["target"] || string parsedJson.["target"] = "fable"
and had missed that it lower cases the string when validating and not when using it (so it seems that setting target to Fable (capital F) with the current release actually causes it to generate a Shared Project (is that considered a bug then?)

* Using `GetAwaiter().GetResult()` for sync sounds good. It doesn't matter that much, both should work fine (if it is easier to leave it as is, I am fine with that)

Doing GetAwaiter().GetResult() directly on the task returned from the Async function should be straight forward, but I had a go at calling it through Task.Run instead and got some overload resolution errors, so sticking to the F# functions which just worked was easier (too many ways of doing the same thing!)

@Zaid-Ajaj
Copy link
Owner Author

and had missed that it lower cases the string when validating and not when using it (so it seems that setting target to Fable (capital F) with the current release actually causes it to generate a Shared Project (is that considered a bug then?)

Yes, that's a bug!

@Numpsy
Copy link
Contributor

Numpsy commented Jun 15, 2021

Considered resolved by merging #47 ?

I'd wondered about adding CancellationToken parameters to the Task returning variants of the functions, but that could be a seperate change if needed.

@Zaid-Ajaj
Copy link
Owner Author

This is resolved and tested locally! thanks a lot @Numpsy ❤️ you can try this out as of v1.26

@Zaid-Ajaj
Copy link
Owner Author

Let's worry about the cancellation token in a different issue 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants