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
272 stars 54 forks source link

Implement char serialization in MsgPack #245

Closed kerams closed 3 years ago

kerams commented 3 years ago

Implements #244.

Characters are converted to a string first in order to get the byte representation. Not the most efficient thing in the world but easy and supposedly rarely used. If anyone complains that their mission-critical systems are failing because of this, I'll find a better solution :).

kerams commented 3 years ago

It works well enough for *, but ŕ is apparently encoded as "Å•" in the input JSON, and request deserialization fails with Newtonsoft.Json.JsonSerializationException: 'Error converting value "☃" to type 'System.Char'. Path '[0].CharValue'.'

Zaid-Ajaj commented 3 years ago

but ŕ is apparently encoded as "Å•" in the input JSON

@kerams You mean from Fable (by SimpleJson or MsgPack) to the backend?

kerams commented 3 years ago

Yes, this throws https://github.com/Zaid-Ajaj/Fable.Remoting/blob/master/Fable.Remoting.Server/Proxy.fs#L98

Zaid-Ajaj commented 3 years ago

Then I think I need to make SimpleJson output the char code instead of the actual string

kerams commented 3 years ago

I think you're just encoding it incorrectly. JSON input supports UTF8, so ŕ should appear as a string of length 1, even though it takes up more than 1 byte when encoded.

Simply {"CharValue": "ŕ" } should do it.

Zaid-Ajaj commented 3 years ago

I will test this out later and see what's going on

Zaid-Ajaj commented 3 years ago

Hi @kerams, so I have been investigating the issue and it seems like the charset encoding of the JSON in the request body wasn't being picked up for some reason. The Content-Type header had value application/json; charset=utf8, I thought at first that it was a typo and changed it to application/json; charset=utf-8 but still no luck. Then I tried adding <meta charset="UTF-8" /> to the HTML page where we run the integration tests and everything works now.

Can you give this another go? The tests should pass now, tbh I don't know if we can do anything about the encoding <meta /> not being present in the HTML

kerams commented 3 years ago

I've cracked it. There's no problem with Content-Type or your bits in SimpleJson. Apparently the HTML meta charset affects the way JS bundles are interpreted. Previously the problem test looked like this in dev tools:

)), qt("IServer.echoRecordWithChar using characters with accents", M.Delay(()=>{
    const e = new kt("Å•");
    return M.Bind(dn.echoRecordWithChar(e), t=>(Sn(e.CharValue, t.CharValue),
    M.Zero()))
}

so SimpleJson is already getting garbage input when creating the request body. With utf-8

)), qt("IServer.echoRecordWithChar using characters with accents", M.Delay(()=>{
    const e = new kt("ŕ");
    return M.Bind(dn.echoRecordWithChar(e), t=>(Sn(e.CharValue, t.CharValue),
    M.Zero()))
}

Nothing to worry about in the end. I think this would work even without the charset if the data was collected from user input, because everything is fine on Remoting's end. The rest is the responsibility the library user.

Most importantly, the IntegrationTestsNagareyamaLive page is now prettier with working icons :)))).

Zaid-Ajaj commented 3 years ago

Great! Nothing to worry about then 😄 Just published a new batch of packages starting from MsgPack and all downstream packages.

I wanted to include a fix for #250 and gave it a go but no luck, got some compile errors and gave up quickly, will try it again soon (unless you want to pick it up, that would be really awesome) 🙏