search

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

minimum code to handle DataTable serialization through BinaryFormatter #203

Closed smoothdeveloper closed 3 years ago

kerams commented 3 years ago

The 2 new !FABLE_COMPILER directives in Write.fs are not needed as you're already inside of a big block that is conditionally compiled on .NET (lines 12 - 433).

Zaid-Ajaj commented 3 years ago

@smoothdeveloper The changes look great! Besides the last comment by @kerams, there is just one more thing: adding an end-to-end test to the communication. This should be easy if you look inside Fable.Remoting.Giraffe.Tests, add a new remote function (record field) to the IBinaryServer protocol like an echo function: echoDataTable : DataTable -> Async<DataTable> and test the round trip in the MiddlewareTests.fs file 🙏

kerams commented 3 years ago

@Zaid-Ajaj, JS <-> .NET integration tests won't work since Fable does not support data tables, right?

Zaid-Ajaj commented 3 years ago

JS <-> .NET integration tests won't work since Fable does not support data tables, right?

@kerams That's right, I think using a DataTable type won't even compile in a Fable app

smoothdeveloper commented 3 years ago

@Zaid-Ajaj I've adressed the review comments, although adding the test in MiddlewareTests.fs gave faulty roundtrip somehow.

@kerams I think I've followed correctly the advice for active pattern, thanks to both for showing me around.

Zaid-Ajaj commented 3 years ago

@smoothdeveloper I am afraid the reason you had failing tests is because the serialization and deserialization is not entirely correct :/ first of all, the TableName isn't going back and forth correctly. This was the original test:

// input table
t.TableName <- "myname"
// assert
Expect.equal deserialized.TableName  t.TableName  "table name"

When I added full roundtrip tests, I though maybe it is just the TableName that was incorrect but actually even the types are deserialized incorrectly, an int column from the input table became an int64 when deserialized. I tried a bunch of combinations to ensure exact conversions but no luck. I think this needs more refinement and maybe a more fledged out intermediate binary representation that includes the table name, schema and data separately from which the data table can be reconstructed on the other end.

I will revert this PR for now, please give it another try on your branch because my attempts have so far failed.

smoothdeveloper commented 3 years ago

@Zaid-Ajaj sorry for you encountering the issues! It has been trial and error but I also noticed the type conversions after the roundtrip in the second test.

I'll try to get it working, there are few bits that need to be taken care of.