Zaid-Ajaj / Fable.Remoting

Type-safe communication layer (RPC-style) for F# featuring Fable and .NET Apps
https://zaid-ajaj.github.io/Fable.Remoting/
MIT License
273 stars 55 forks source link

Client always sends null input parameter to functions with return type Async<Unit> #135

Closed ScottShingler closed 5 years ago

ScottShingler commented 5 years ago

Given the following API definition:

type myApi =
    { doSomething: int -> Async<Unit>
      returnSomething: int -> Async<int> }

And the following client-side invocation:

type Msg =
    | DoSomething of int
    | DidSomething of int
    | ReturnSomething of int
    | ReturnedSomething of int
    | Error of Exception

let update msg model : Model * Cmd<Msg> =
    match msg with
    | DoSomething x ->
        model,
        // Problem occurs here
        Cmd.OfAsync.either Server.api.doSomething x (fun _ -> DidSomething fileId) Error

    | DidSomething x ->
        ...

    | ReturnSomething x ->
        model,
        Cmd.OfAsync.either Server.api.returnSomething x ReturnedSomething Error

    | ReturnedSomething x ->
        ...

    | Error ->
        model, Cmd.none

Upon processing the DoSomething message, the client will POST to the doSomething endpoint with a payload of [null].

On the server, I see the following:

info: Microsoft.AspNetCore.Hosting.Internal.WebHost[1]
      Request starting HTTP/1.1 POST http://localhost:8085/api/myApi/doSomething application/json;charset=UTF-8 6
Fable.Remoting:
About to deserialize JSON:

Into .NET Types:
[int]

Newtonsoft.Json.JsonSerializationException: Error converting value {null} to type 'System.Int32'. Path '[0]'. ---> System.InvalidCastException: Null object cannot be converted to a value type.
   at System.Convert.ChangeType(Object value, Type conversionType, IFormatProvider provider)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.EnsureType(JsonReader reader, Object value, CultureInfo culture, JsonContract contract, Type targetType)
   --- End of inner exception stack trace ---
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.EnsureType(JsonReader reader, Object value, CultureInfo culture, JsonContract contract, Type targetType)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.Deserialize(JsonReader reader, Type objectType, Boolean checkAdditionalContent)
   at Newtonsoft.Json.JsonSerializer.DeserializeInternal(JsonReader reader, Type objectType)
   at Newtonsoft.Json.Linq.JToken.ToObject(Type objectType, JsonSerializer jsonSerializer)
   at Fable.Remoting.Server.DynamicRecord.createArgsFromJson$cont@132-1(RecordFunctionInfo func, FSharpOption`1 logger, Type input, JToken parsedJson, Unit unitVar)
   at Fable.Remoting.Server.DynamicRecord.tryCreateArgsFromJson(RecordFunctionInfo func, String inputJson, FSharpOption`1 logger)
Fable.Remoting: Returning serialized result back to client
{"ParsingArgumentsError":"Error converting value {null} to type 'System.Int32'. Path '[0]'."}

In the transpiled JavaScript, I see that the doSomething function has a hardcoded null parameter.

However, when the client processes the ReturnSomething message, everything works as expected.

My Fable.Remoting versions are as follows:

Fable.Remoting.Client (5.6)
Fable.Remoting.Giraffe (3.8)
Fable.Remoting.Json (2.4)
Fable.Remoting.Server (4.8)

Am I doing something wrong, or does Fable.Remoting not support functions with a return type of Async<Unit>?

Zaid-Ajaj commented 5 years ago

Hello @ScottShingler, thanks a lot for filing the issue, I will look into it very soon in the next few day. I was pretty sure that returning Async<unit> was supported so it could be a currying issue, can you try using the following:

Cmd.OfAsync.either (fun value -> Server.api.doSomething value) x (fun _ -> DidSomething fileId) Error

Also would be great if you have a sample repo where I could reproduce the problem :)

ScottShingler commented 5 years ago

Hi @Zaid-Ajaj, thanks for the lightning-fast reply! I tried your suggestion, but the behaviour remains the same. I will try to set up a repro repo.

Zaid-Ajaj commented 5 years ago

Awesome, thanks a lot!

ScottShingler commented 5 years ago

@Zaid-Ajaj, I have created a repro here: https://github.com/ScottShingler/Fable.Remoting-Issue-135-Repro.

Also, I realized I have a small typo in my example above:

        // Problem occurs here
        Cmd.OfAsync.either Server.api.doSomething x (fun _ -> DidSomething fileId) Error

fileId should have been x:

        // Problem occurs here
        Cmd.OfAsync.either Server.api.doSomething x (fun _ -> DidSomething x) Error

I don't believe the typo has any bearing on the issue. I hope the repro will be useful.

ScottShingler commented 5 years ago

Something interesting I observed - it appears to be related somehow to the paket dependencies. If I create a new SAFE project using dotnet new SAFE -la none -c remoting and set up the API, it works correctly. However, if I modify paket.dependencies as I have in the repro, it seems to bring about the error.

Zaid-Ajaj commented 5 years ago

Hmm it could be that the reflection type information is not updated when you add new packages so you have to reset some cache by deleting obj en bin directories, then restarting the build, would you please give that a try?

ScottShingler commented 5 years ago

I tried clearing out cached files, but the behaviour stays the same.

In my previous comment, I mentioned how my code running in a project created with dotnet new SAFE -la none -c remoting works correctly. It turns out if I simply delete the paket.lock file and run fake build -t run, the problem appears.

I have updated my repro to illustrate this. Please try it out and let me know if you observe the same behaviour.

Zaid-Ajaj commented 5 years ago

@ScottShingler I am indeed to reproduce the problem and it looks like a Fable issue, if you change the update function to the following:

let update (msg : Msg) (model : Model) : Model * Cmd<Msg> =
    match msg with
    | DoSomething x ->
        { Message = "Doing something..." },
        Cmd.OfAsync.either (fun () -> Server.api.doSomething x) () (fun _ -> DidSomething) Error

    | DidSomething ->
        { Message = "Success!"}, Cmd.none

    | Error e->
        { Message = sprintf "Error: %O" e}, Cmd.none

Then it works! Notice that I changed the way you call Server.api.doSomething so it is not really a Fable.Remoting issue, I will try to make a small repro and re-issue this on Fable repo, meanwhile I am keeping this one open :smile: Thanks a lot for your feedback

Zaid-Ajaj commented 5 years ago

OK, I am not able to reproduce this anywhere else other than your repro code 😓 it might just the wrong combination of old packages and Fable not able to create the write closure. I will write some tests for Async<unit> (I thought I had some) to make sure the code works as expected as of the latest versions.

ScottShingler commented 5 years ago

@Zaid-Ajaj Thank you for taking the time to investigate this. Indeed, your suggestion to change how I call Server.api.doSomething works.

Could you post the contents of your paket.dependencies and paket.lock files? It could help me to figure out where my versions are not lining up.

Zaid-Ajaj commented 5 years ago

My lock file is the same one you gave me in the reproduction project which indeed latest versions of Fable.Remoting stuff, I even updated to latest Fable compiler/splitter/loader and concluded that the problem isn't related to Fable.Remoting but it is a Fable bug, if you change your Server module to the following, then it still fails

// THIS FAILS
module Server =
    let api = {|
        doSomething = fun (n: int) -> async {
            do! Async.Sleep 1000
            printf "%s" (n.ToString())
            return ()
        }
    |}

then the given argument to doSomething stays null unless I change the return type from Async<unit> to anything else like Async<string>, then it works just fine

// THIS WORKS
module Server =
    let api = {|
        doSomething = fun (n: int) -> async {
            do! Async.Sleep 1000
            printf "%s" (n.ToString())
            return "whatever"
        }
    |}

I will close this issue here and make a repro for Fable, this is some really strange bug 😅