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

Fable 4.0.6 breaks reflection for single-case unions #3418

Closed
OnurGumus opened this issue Apr 12, 2023 · 19 comments
Closed

Fable 4.0.6 breaks reflection for single-case unions #3418

OnurGumus opened this issue Apr 12, 2023 · 19 comments

Comments

@OnurGumus
Copy link

Description

It is is hard to be a bit specific for my case because code isn't public. But I have some code using Fable.Remoting. It fetches some data from server via Fable.Remoting and Elmish commands. In 4.0.5 compiler it works fine. When I use 4.0.6 it returns all zeroes without any errors

@ncave
Copy link
Collaborator

ncave commented Apr 12, 2023

@OnurGumus Is it possible to check if both ends are using Fable 4.0.6? (I'm not saying they should, just clarifying for context).

@alfonsogarciacaro Possibly somehow related to #3412 (or not, see also issue #3419).
Does the JSON serialization component need a new version?

@OnurGumus
Copy link
Author

OnurGumus commented Apr 12, 2023

The other side is a giraffe server. So no Fable there. When I say it uses Fable 4.0.6, I refer to the dotnet tool. And I verified server response is correct.

The interesting bit is the data I was receving is int64 and they are coming 0 instead of real value. Reverting 4.0.5 from dotnet tool fixes the problem.

@OnurGumus OnurGumus changed the title Fable 4.0.6 breaks Fable.Remoting deserialization Fable 4.0.6 breaks Fable.Remoting deserialization such as int64 values become 0 Apr 12, 2023
@ncave
Copy link
Collaborator

ncave commented Apr 12, 2023

@OnurGumus The int64 change to native JS bigint was introduced in 4.0.5.
If it works with 4.0.5, then it's probably something else then, related to 4.0.6.

I don't suppose you can provide a snippet or sample of the server's response json?
What is being used for JSON serialization on the server, and deserialization on the client?

@ncave
Copy link
Collaborator

ncave commented Apr 12, 2023

@OnurGumus

I verified server response is correct.

Then it must be the JSON deserialization, if you can provide a snippet of what that int64 looks like in the response JSON, we'll be able to see what the problem is.

@OnurGumus
Copy link
Author

OnurGumus commented Apr 12, 2023

In my case it looks like below

{"VisitorId":{"VisitorId":"tb4q8ARZSVCrFucPXchi"},"Credit":{"Credit":"+782"},"Version":{"Version":"+85"}}

@Zaid-Ajaj
Copy link
Member

@OnurGumus the issue might be in Fable.SimpleJson which handles the deserialization business on the client side of things, used by Fable.Remoting.Client.

@OnurGumus
Copy link
Author

Perhaps, but the fact is ,it works well with all fable but 4.06

@ncave
Copy link
Collaborator

ncave commented Apr 12, 2023

@OnurGumus Is Fable.SimpleJson being used for serialization on both ends?

Standard Giraffe JSON serialization looks a bit different for int64:

{"text":"0x400000000000000","int64":288230376151711744}

@alfonsogarciacaro
Copy link
Member

Before we had a toJson method in Long.js prototype (which is called if present by JSON.stringify).

Long.prototype.toJSON = function () { return toString(this); }

Should we add it to BigInt prototype? I think it's not recommended to modify the prototype of native objects, but here it's proposed as solution.
https://stackoverflow.com/a/70315718

@ncave
Copy link
Collaborator

ncave commented Apr 13, 2023

@alfonsogarciacaro That's already there. We just need to understand what component is used to deserialize and does it need to be updated to a new version. As described above, presumably 4.0.5 works, so it's not the bigint, it's something introduced in 4.0.6.

@OnurGumus
Copy link
Author

OnurGumus commented Apr 13, 2023

I have reconfirmed, it works with 4.0.5 and gets broken with 4.0.6. It uses Fable.SimpleJson on the client to deserialize. It actually seems initial deserialization is correct. Then goes back to elmish async and I lose track. See the screenshot

image

Though at this point it is still not converted to an int yet.

@kerams
Copy link
Contributor

kerams commented Apr 13, 2023

I'm on 4.0.6 and at first glance both JSON and MsgPack serialization of longs via Remoting look OK.

@OnurGumus
Copy link
Author

OnurGumus commented Apr 13, 2023

Actually I believe this issue has nothing to do with int64. I can see entire data is deserialized to empty.
By empty I refer to all fields being 0

@OnurGumus OnurGumus changed the title Fable 4.0.6 breaks Fable.Remoting deserialization such as int64 values become 0 Fable 4.0.6 breaks Fable.Remoting deserialization Apr 13, 2023
@ncave
Copy link
Collaborator

ncave commented Apr 13, 2023

@OnurGumus

It actually seems initial deserialization is correct.
Though at this point it is still not converted to an int yet.

If not the initial deserialization, then what converts those fields to int64?
If we can narrow that down, hopefully we can see where it breaks and why.

@OnurGumus
Copy link
Author

That’s something @Zaid-Ajaj can answer perhaps.

@alfonsogarciacaro
Copy link
Member

Thanks a lot you all for the investigation! I think I know where the problem is. @OnurGumus Is the parsed type (or one of the nested values) a single-case union?

@OnurGumus
Copy link
Author

OnurGumus commented Apr 15, 2023

@alfonsogarciacaro Yes. To be precise it looks as below:

type VisitorId = VisitorId of string

type Credit = Credit of int64
type Version = Version of int64

type CreditData = { VisitorId: VisitorId; Credit: Credit; Version : Version }

@alfonsogarciacaro alfonsogarciacaro changed the title Fable 4.0.6 breaks Fable.Remoting deserialization Fable 4.0.6 breaks reflection for single-case unions Apr 16, 2023
@alfonsogarciacaro
Copy link
Member

@OnurGumus Can you please try with Fable 4.1.0-beta-001?

@OnurGumus
Copy link
Author

@alfonsogarciacaro problem solved with 4.1.0-beta-001. Thanks for excellent support!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants