Added generation GraphQLClient.fsi #49

anthony-mi · 2021-06-17T18:57:36Z

No description provided.

Zaid-Ajaj · 2021-06-17T19:27:02Z

src/CodeGen.fs

+        let options =
+            let options' = JsonSerializerOptions()
+            options'.Converters.Add(JsonFSharpConverter())
+            options'
+        {clientName}(url, new HttpClient(), options)


Can you change to the following:

let options = JsonSerializerOptions() options.Converters.Add(JsonFSharpConverter()) {clientName}(url, new HttpClient(), options)

@Zaid-Ajaj updated

xperiandri · 2021-06-17T20:24:39Z

src/CodeGen.fs

+type {clientName} =
+    class
+        /// <summary>Creates {clientName}</summary>


Suggested change

/// <summary>Creates {clientName}</summary>

/// <summary>Creates {clientName} specifying <see href="T:HttpClient">HttpClient</see> instance</summary>

xperiandri · 2021-06-17T20:25:42Z

src/CodeGen.fs

+        /// <see href="T:Fable.Remoting.Json.FableJsonConverter">FableJsonConverter</see>
+        /// from <a href="https://github.com/Zaid-Ajaj/Fable.SimpleJson">Fable.SimpleJson</a> NuGet package


Suggested change

/// In order to enable all F# types serialization and deserealization you must add

/// <see href="T:Fable.Remoting.Json.FableJsonConverter">FableJsonConverter</see>

/// from <a href="https://github.com/Zaid-Ajaj/Fable.SimpleJson">Fable.SimpleJson</a> NuGet package

/// In order to enable all F# types serialization and deserealization

/// <see href="T:Fable.Remoting.Json.FableJsonConverter">FableJsonConverter</see> is added

/// from <a href="https://github.com/Zaid-Ajaj/Fable.SimpleJson">Fable.SimpleJson</a> NuGet package

These remarks are not entirely correct, the type FableJsonConverter from Fable.Remoting.Json is not from Fable.SimpleJson.

Fable.Remoting.Json is a F# JSON converter running on dotnet

Fable.SimpleJson is F# JSON converter running with Fable

xperiandri · 2021-06-17T20:28:16Z

src/CodeGen.fs

+        /// </remarks>
+        /// <param name="url">GraphQL endpoint URL</param>
+        new: url: string * client: HttpClient -> SpotifyGraphqlClient        new: string * HttpClient * JsonSerializerOptions -> SpotifyGraphqlClient


Suggested change

new: url: string * client: HttpClient -> SpotifyGraphqlClient new: string * HttpClient * JsonSerializerOptions -> SpotifyGraphqlClient

new: url: string * client: HttpClient -> SpotifyGraphqlClient

/// <summary>Creates {clientName}</summary>

/// <remarks>

/// In order to enable all F# types serialization and deserealization

/// <see href="T:Fable.Remoting.Json.FableJsonConverter">FableJsonConverter</see> is added

/// from <a href="https://github.com/Zaid-Ajaj/Fable.SimpleJson">Fable.SimpleJson</a> NuGet package

/// </remarks>

new: url: string -> SpotifyGraphqlClient

xperiandri · 2021-06-17T20:30:28Z

src/CodeGen.fs

+type {clientName} =
+    class
+        /// <summary>Creates {clientName}</summary>


Suggested change

/// <summary>Creates {clientName}</summary>

/// <summary>

/// Creates {clientName} specifying <see href="T:HttpClient">HttpClient</see> instance

/// and <see href="T:JsonSerializerOptions">JsonSerializerOptions</see>

/// </summary>

xperiandri · 2021-06-17T20:35:35Z

src/CodeGen.fs

    new(url: string, options: JsonSerializerOptions) = {clientName}(url, new HttpClient(), options)
    new(url: string) = {clientName}(url, new HttpClient(), new JsonSerializerOptions())
+    new(url: string)=
+        let options = JsonSerializerOptions()
+        options.Converters.Add(JsonFSharpConverter())
+        {clientName}(url, new HttpClient(), options)


Suggested change

new(url: string, options: JsonSerializerOptions) = {clientName}(url, new HttpClient(), options)

new(url: string) = {clientName}(url, new HttpClient(), new JsonSerializerOptions())

new(url: string)=

let options = JsonSerializerOptions()

options.Converters.Add(JsonFSharpConverter())

{clientName}(url, new HttpClient(), options)

new(url: string, options: JsonSerializerOptions) = {clientName}(url, new HttpClient(), options)

new(url: string, client: HttpClient) =

let options = JsonSerializerOptions()

options.Converters.Add(JsonFSharpConverter())

{clientName}(url, client, options)

new(url: string) = {clientName}(url, new HttpClient())

xperiandri · 2021-06-17T20:38:08Z

src/CodeGen.fs

+        /// </remarks>
+        /// <param name="url">GraphQL endpoint URL</param>
+        new: url: string * client: HttpClient * options: JsonSerializerOptions -> SpotifyGraphqlClient        new: string * HttpClient * JsonSerializerOptions -> SpotifyGraphqlClient


Suggested change

new: url: string * client: HttpClient * options: JsonSerializerOptions -> SpotifyGraphqlClient new: string * HttpClient * JsonSerializerOptions -> SpotifyGraphqlClient

new: url: string * client: HttpClient * options: JsonSerializerOptions -> SpotifyGraphqlClient

/// <summary>

/// Creates {clientName} specifying <see href="T:JsonSerializerOptions">JsonSerializerOptions</see>

/// </summary>

/// <param name="url">GraphQL endpoint URL</param>

/// <remarks>

/// In order to enable all F# types serialization and deserealization <b>you must</b> add

/// <see href="T:System.Text.Json.Serialization.JsonFSharpConverter">JsonFSharpConverter</see>

/// from <a href="https://github.com/Tarmil/FSharp.SystemTextJson">FSharp.SystemTextJson</a> NuGet package yourself

/// </remarks>

new: url: string * options: JsonSerializerOptions -> SpotifyGraphqlClient

/// <summary>

/// Creates {clientName} specifying <see href="T:HttpClient">HttpClient</see> instance

/// </summary>

/// <param name="url">GraphQL endpoint URL</param>

/// <remarks>

/// In order to enable all F# types serialization and deserealization

/// <see href="T:System.Text.Json.Serialization.JsonFSharpConverter">JsonFSharpConverter</see> by default is added

/// from <a href="https://github.com/Tarmil/FSharp.SystemTextJson">FSharp.SystemTextJson</a> NuGet package

/// </remarks>

new: url: string * client: HttpClient -> SpotifyGraphqlClient

/// <summary>Creates {clientName}</summary>

/// <param name="url">GraphQL endpoint URL</param>

/// <remarks>

/// In order to enable all F# types serialization and deserealization

/// <see href="T:System.Text.Json.Serialization.JsonFSharpConverter">JsonFSharpConverter</see> by default is added

/// from <a href="https://github.com/Tarmil/FSharp.SystemTextJson">FSharp.SystemTextJson</a> NuGet package

/// </remarks>

new: url: string -> SpotifyGraphqlClient

xperiandri · 2021-06-17T20:42:48Z

src/CodeGen.fs

+        /// <see href="T:System.Text.Json.Serialization.JsonFSharpConverter">JsonFSharpConverter</see>
+        /// from <a href="https://github.com/Tarmil/FSharp.SystemTextJson">FSharp.SystemTextJson</a> NuGet package


Suggested change

/// In order to enable all F# types serialization and deserealization you must add

/// <see href="T:System.Text.Json.Serialization.JsonFSharpConverter">JsonFSharpConverter</see>

/// from <a href="https://github.com/Tarmil/FSharp.SystemTextJson">FSharp.SystemTextJson</a> NuGet package

/// In order to enable all F# types serialization and deserealization <b>you must</b> add

/// <see href="T:System.Text.Json.Serialization.JsonFSharpConverter">JsonFSharpConverter</see>

/// from <a href="https://github.com/Tarmil/FSharp.SystemTextJson">FSharp.SystemTextJson</a> NuGet package yourself

xperiandri · 2021-06-17T20:44:39Z

src/CodeGen.fs

+    class
+        /// <summary>Creates {clientName}</summary>
+        /// <remarks>
+        /// In order to enable all F# types serialization and deserealization you must add 
+        /// <see href="T:Fable.Remoting.Json.FableJsonConverter">FableJsonConverter</see>
+        /// from <a href="https://github.com/Zaid-Ajaj/Fable.SimpleJson">Fable.SimpleJson</a> NuGet package
+        /// </remarks>
+        /// <param name="url">GraphQL endpoint URL</param>
+        new: url: string * client: HttpClient * options: JsonSerializerOptions -> SpotifyGraphqlClient        new: string * HttpClient * JsonSerializerOptions -> SpotifyGraphqlClient
+    end


Analogical to previous

Zaid-Ajaj · 2021-06-18T00:31:31Z

src/Program.fs

                colorprintfn "✏️  Generating GraphQL client $green[%s]" graphqlClientPath
-                generatedFiles.Add(graphqlClientPath)
+                generatedFiles.AddRange([| graphqlClientPath; graphqlClientFsiPath|])


the fsi file must be compiled before the the actual implementation file:

generatedFiles.AddRange [| graphqlClientFsiPath graphqlClientPath |]

Zaid-Ajaj · 2021-06-18T00:32:37Z

src/Program.fs

+                        seq {
+                            MSBuildXElement.Compile($".\{config.project}.GraphqlClient.fs")
+                            MSBuildXElement.Compile($".\{config.project}.GraphqlClient.fsi")
+                        }
+                    else
+                        seq {
+                            MSBuildXElement.Compile($".\{outputDirectoryName}\\dotnet\{config.project}.GraphqlClient.fs")
+                            MSBuildXElement.Compile($".\{outputDirectoryName}\\dotnet\{config.project}.GraphqlClient.fsi")
+                        }


Same as above, the fsi definition comes before the implementation

Zaid-Ajaj · 2021-06-18T00:34:50Z

src/CodeGen.fs

+    | SerializerType.Newtonsoft -> sampleFSharpNewtonsoftGraphqlClient projectName clientName errorType members
+
+let sampleFableGraphqlClientFsi projectName clientName =


This function is implemented incorrectly, Fable fsi implementation doesn't use any of Newtonsoft.Json, Fable.Remoting.Json, System.Net.Http and definitely no HttpClient

@Zaid-Ajaj updated

xperiandri · 2021-06-19T14:17:34Z

@Zaid-Ajaj can I use FusionTasks NuGet to simplify code?

task computation expression does not require Async.AwaitTask, async computation expression requires it.
With FusionTasks we can use the same code and simplify generation

Zaid-Ajaj · 2021-06-19T20:57:59Z

@xperiandri task is already included when asyncReturnType: "task" in which case the Ply package is included

xperiandri · 2021-06-19T20:59:28Z

I need https://www.nuget.org/packages/FSharp.Control.FusionTasks/2.2.0

Zaid-Ajaj · 2021-06-20T12:15:45Z

You mean you want to use FusionTasks to simplify the async implementation? I don't really think adding a dependency just for Async.AwaitTask is worth it.

If you need FusionTasks in your application, why not simply add it in your project that references the generated Snowflaqe client?

xperiandri · 2021-06-20T15:12:26Z

It worse because now we have 2 generator functions everywhere async/task appear which complecates the code a lot and adds more chances for mistake

Zaid-Ajaj · 2021-06-20T15:24:58Z

I agree it is a bit complicated but it makes the result nicer for users who want Task<'T> as the output of the client function, not just for simplifying the implementation details.

I think implementing error handling option from #29 will be the last hurdle and it's still doable IMO

Do you have any other features might be impacted because of the task/option?

xperiandri · 2021-06-20T21:23:17Z

I don't know why could people not use https://www.nuget.org/packages/FSharp.Control.FusionTasks/2.2.0
It is one of the first NuGet packages I install to any F# project. Adding Async.AwaitTask to every async .NET BCL call is crazy...

xperiandri · 2021-06-20T21:25:18Z

The same as manually calling OnPropertyChanged instead of installing PropertyChanged.Fody and make it done automatically

Zaid-Ajaj · 2021-06-20T22:27:32Z

I don't know why could people not use https://www.nuget.org/packages/FSharp.Control.FusionTasks/2.2.0
It is one of the first NuGet packages I install to any F# project. Adding Async.AwaitTask to every async .NET BCL call is crazy...

Users of Snowflaqe don't have to call Async.AwaitTask, it is just the generated code. You can always install FusionTasks into your own projects whenever you need it.

Sticking with Ply for now as a dependency of the generated projects because it gives a nice task CE to work with when tasks are preferred.

Numpsy · 2021-06-21T10:01:58Z

Sticking with Ply for now as a dependency of the generated projects because it gives a nice task CE to work with when tasks are preferred.

One of the reasons I made the task change was that 'fits' more when the actual 'async' functionality is using tasks anyway (i.e. HttpClient and the Json libs are task based), but then I was also having a go at inserting the generated lib into an existing C# stack and the tasks returned from Ply just work directly there which is nice for interop.

(I'm a massive beginner with F# and had never heard of FusionTasks, but it looks to me like task { } is going to end up part of FSharp Core at some point, at which point the issue and the requirement for 3rd party libs might just go away? (or maybe i'm being hopeful looking at how long that question has been running for!))

Zaid-Ajaj · 2021-07-12T10:00:53Z

Hi @anthony-mi what's the status of this PR? Any plans to continue working on?

@xperiandri if there are issues or discussions outside of this PR, please consider opening a separate issue to discuss it there

xperiandri · 2021-07-12T12:29:48Z

@Zaid-Ajaj Anthony decided to exit from the project. The other developer will pick up the work in several weeks.

Zaid-Ajaj · 2021-07-12T14:07:45Z

Thanks for the update @xperiandri does this mean this PR can closed to start all over later on or should I keep it open for now?

xperiandri · 2021-07-12T14:08:37Z

Keep it open for now. I will let you know

xperiandri · 2021-09-30T08:43:31Z

@Zaid-Ajaj we have discovered that in order to have FSI we need to declare all public members within it too

xperiandri · 2021-09-30T08:46:13Z

So that we think of removing FSI and rewriting the client class without the default constructor

Zaid-Ajaj · 2021-09-30T08:49:17Z

So that we think of removing FSI and rewriting the client class without the default constructor

Sounds like a good idea for now. Proper FSI support can be added in another PR

xperiandri · 2021-09-30T09:48:34Z

We will have a look closer soon. Removing FSI looks easier to implement an maintain for now

Zaid-Ajaj reviewed Jun 17, 2021

View reviewed changes

anthony-mi force-pushed the FSharp_SystemTextJson branch from 0ddabea to dfe260b Compare June 17, 2021 20:05

xperiandri suggested changes Jun 17, 2021

View reviewed changes

anthony-mi force-pushed the FSharp_SystemTextJson branch from dfe260b to f23c7a9 Compare June 17, 2021 23:05

Zaid-Ajaj reviewed Jun 18, 2021

View reviewed changes

anthony-mi force-pushed the FSharp_SystemTextJson branch 3 times, most recently from 8fc4e93 to aeb9b29 Compare June 18, 2021 20:05

Added generation GraphQLClient.fsi

fd8d5a8

anthony-mi force-pushed the FSharp_SystemTextJson branch from aeb9b29 to fd8d5a8 Compare June 19, 2021 00:35

Zaid-Ajaj merged commit fd8d5a8 into Zaid-Ajaj:master Sep 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added generation GraphQLClient.fsi #49

Added generation GraphQLClient.fsi #49

anthony-mi commented Jun 17, 2021

Zaid-Ajaj Jun 17, 2021

anthony-mi Jun 17, 2021 •

edited

Loading

xperiandri Jun 17, 2021

xperiandri Jun 17, 2021

Zaid-Ajaj Jun 18, 2021

xperiandri Jun 17, 2021

xperiandri Jun 17, 2021

xperiandri Jun 17, 2021

xperiandri Jun 17, 2021

xperiandri Jun 17, 2021

xperiandri Jun 17, 2021

Zaid-Ajaj Jun 18, 2021

Zaid-Ajaj Jun 18, 2021

Zaid-Ajaj Jun 18, 2021

anthony-mi Jun 18, 2021

xperiandri commented Jun 19, 2021

Zaid-Ajaj commented Jun 19, 2021

xperiandri commented Jun 19, 2021

Zaid-Ajaj commented Jun 20, 2021

xperiandri commented Jun 20, 2021

Zaid-Ajaj commented Jun 20, 2021

xperiandri commented Jun 20, 2021

xperiandri commented Jun 20, 2021

Zaid-Ajaj commented Jun 20, 2021

Numpsy commented Jun 21, 2021

Zaid-Ajaj commented Jul 12, 2021

xperiandri commented Jul 12, 2021

Zaid-Ajaj commented Jul 12, 2021

xperiandri commented Jul 12, 2021

xperiandri commented Sep 30, 2021

xperiandri commented Sep 30, 2021

Zaid-Ajaj commented Sep 30, 2021

xperiandri commented Sep 30, 2021

	/// <summary>Creates {clientName}</summary>
	/// <summary>Creates {clientName} specifying <see href="T:HttpClient">HttpClient</see> instance</summary>

		/// <see href="T:Fable.Remoting.Json.FableJsonConverter">FableJsonConverter</see>
		/// from <a href="https://github.com/Zaid-Ajaj/Fable.SimpleJson">Fable.SimpleJson</a> NuGet package

-        /// <summary>Creates {clientName}</summary>
+        /// <summary>
+        /// Creates {clientName} specifying <see href="T:HttpClient">HttpClient</see> instance
+        /// and <see href="T:JsonSerializerOptions">JsonSerializerOptions</see>
+        /// </summary>

		/// <see href="T:System.Text.Json.Serialization.JsonFSharpConverter">JsonFSharpConverter</see>
		/// from <a href="https://github.com/Tarmil/FSharp.SystemTextJson">FSharp.SystemTextJson</a> NuGet package

		\| SerializerType.Newtonsoft -> sampleFSharpNewtonsoftGraphqlClient projectName clientName errorType members

		let sampleFableGraphqlClientFsi projectName clientName =

Added generation GraphQLClient.fsi #49

Added generation GraphQLClient.fsi #49

Conversation

anthony-mi commented Jun 17, 2021

Choose a reason for hiding this comment

anthony-mi Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xperiandri commented Jun 19, 2021

Zaid-Ajaj commented Jun 19, 2021

xperiandri commented Jun 19, 2021

Zaid-Ajaj commented Jun 20, 2021

xperiandri commented Jun 20, 2021

Zaid-Ajaj commented Jun 20, 2021

xperiandri commented Jun 20, 2021

xperiandri commented Jun 20, 2021

Zaid-Ajaj commented Jun 20, 2021

Numpsy commented Jun 21, 2021

Zaid-Ajaj commented Jul 12, 2021

xperiandri commented Jul 12, 2021

Zaid-Ajaj commented Jul 12, 2021

xperiandri commented Jul 12, 2021

xperiandri commented Sep 30, 2021

xperiandri commented Sep 30, 2021

Zaid-Ajaj commented Sep 30, 2021

xperiandri commented Sep 30, 2021

anthony-mi Jun 17, 2021 •

edited

Loading