-
Notifications
You must be signed in to change notification settings - Fork 57
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
Fixes #226 and Fixes #227 #228
Conversation
let kindAsInt = x.Read typeof<int64> :?> int64 | ||
let kind = | ||
match kindAsInt with | ||
| 1L -> DateTimeKind.Utc | ||
| 2L -> DateTimeKind.Local | ||
| _ -> DateTimeKind.Unspecified |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enums are supported so you can simply do
let kind = x.Read typeof<DateTimeKind> :?> DateTimeKind
and on the .NET write side (might have to move writeEnum
higher up)
writeEnum dt.Kind out
It might be ever so slightly slower than what you did with explicit integer to enum mapping. Up to you ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no problem with using writeInt64
. It won't write more bytes than necessary for each individual integer (could be 1, 2, 3, 5, or 9 bytes). In this case 1 byte, because all DateTimeKind
values are in the range of fixnum.
Or you could also choose to directly write the enum as fixnum for extra speed using out.WriteByte (Format.fixposnum (int dt.Kind))
since we can be sure of this enum's range. I used it for union tags
Fable.Remoting/Fable.Remoting.MsgPack/Write.fs
Lines 254 to 259 in c65f604
let inline writeUnion union (out: Stream) (caseSerializers: Action<'a, Stream>[][]) tagReader = | |
let tag = tagReader union | |
let fieldSerializers = caseSerializers.[tag] | |
out.WriteByte (Format.fixarr 2uy) | |
out.WriteByte (Format.fixposnum tag) |
Yes, it will go up in flames as soon as someone tries to deserialize a union with more than 127 cases (and they'd deserve it! 😂).
Thanks a lot for the explanation and for reviewing @kerams if the way I handled DateTimeKind isn't problematic and works correctly, I will leave it as is. Let's get this in and publish a new batch 😄 |
Unfortunately, MsgPack also depends on TypeShape, so it'll need to be included there too. |
This PR includes TypeShape source file and removes the problematic code that checks for
IsByRefLike
dynamically since we don't need it afaik (fixes #226) I've also attempted to solve #227 where DateTimeKind wasn't preserved when sent over the wire (also because I wanted to dive into the MsgPack implementation a little bit)@kerams Can you please have a look just to make sure I didn't do anything too dumb? Besides that I used the space of a
int64
to send a enum which can only have values {0, 1, 2} 😅