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

Add section on SignalR MessagePack "known issues" #11170

Merged
merged 4 commits into from
Mar 5, 2019

Conversation

analogrelay
Copy link
Contributor

@analogrelay analogrelay commented Feb 27, 2019

There are some known issues/quirks in the MessagePack Hub Protocol for SignalR. We should call them out in our docs instead of making users search GitHub for these answers :).

Internal Review Version

There are some known issues/quirks in the MessagePack Hub Protocol for SignalR. We should call them out in our docs instead of making users search GitHub for these answers :)
@@ -94,6 +94,64 @@ const connection = new signalR.HubConnectionBuilder()
> [!NOTE]
> At this time, there are no configuration options for the MessagePack protocol on the JavaScript client.

## MessagePack Quirks
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure "quirks" is the right name here... very open to suggestions. I hestitate to use "Known Issues" because that (to me) implies that we're going to work on fixing them, which we aren't. I started with "Differences between MessagePack and JSON serialization" but some of these aren't exactly "differences" between JSON and MessagePack, they're just notable things for MessagePack (AOT support, for example didn't feel like a "difference"... though it kind of is... 🤷‍♂️).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think "quirks" describes it well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use sentence casing for headings. For example, "MessagePack quirks".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, wasn't sure if there's a translation concern around using a word like "quirks". I'll update the casing.


### DateTime.MinValue is not supported by MessagePack in JavaScript

The [msgpack5](https://github.com/mcollina/msgpack5) library used by the SignalR JavaScript client does not support the `timestamp96` type in MessagePack. This type is used to encode very large date values (either very early in the past or very far in the future). The value of `DateTime.MinValue` is `January 1, 0001` which must be encoded in a `timestamp96` value. Because of this, sending `DateTime.MinValue` to a JavaScript client is not supported. Usually, `DateTime.MinValue` is used to encode a "missing" or `null` value. If you need to encode that value in MessagePack, use a nullable `DateTime` value (`DateTime?`) or encode a separate `bool` value indicating if the date is present.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of this, sending DateTime.MinValue to a JavaScript client is not supported.

If I tried to send DateTime.MinValue to the JS client with MessagePack, what would happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exception shows up in the JS console:

Uncaught Error: unable to find ext type 255 at decoder.js:427
at h (decoder.js:427)
at l (decoder.js:263)
at d (decoder.js:358)
at l (decoder.js:243)
at u (decoder.js:335)
at l (decoder.js:285)
at u (decoder.js:335)
at l (decoder.js:285)
at u (decoder.js:335)
at l (decoder.js:285)

See aspnet/SignalR#2228

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could include that text in the doc for google-fu


The [msgpack5](https://github.com/mcollina/msgpack5) library used by the SignalR JavaScript client does not support the `timestamp96` type in MessagePack. This type is used to encode very large date values (either very early in the past or very far in the future). The value of `DateTime.MinValue` is `January 1, 0001` which must be encoded in a `timestamp96` value. Because of this, sending `DateTime.MinValue` to a JavaScript client is not supported. Usually, `DateTime.MinValue` is used to encode a "missing" or `null` value. If you need to encode that value in MessagePack, use a nullable `DateTime` value (`DateTime?`) or encode a separate `bool` value indicating if the date is present.

See [aspnet/SignalR#2228](https://github.com/aspnet/SignalR/issues/2228) for more information on this limitation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it recommended to link to GitHub issues for complex topics? Maybe the prose should mention this is a GitHub issue incase some peopled don't recognize the "aspnet/SignalR#2228" GitHub link syntax.

We should probably make sure the issue is locked before linking to it in docs, but it doesn't matter in this particular case, since aspnet/SignalR is archived.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm not sure either. I'm happy to remove it but I thought it might be worth including the link to the issue for more context. Agreed that we can make it clearer that it's an issue link.

Copy link
Member

@scottaddie scottaddie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See feedback below. Also, update the value of the ms.date metadata field to today's date.

@@ -94,6 +94,64 @@ const connection = new signalR.HubConnectionBuilder()
> [!NOTE]
> At this time, there are no configuration options for the MessagePack protocol on the JavaScript client.

## MessagePack Quirks
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use sentence casing for headings. For example, "MessagePack quirks".

aspnetcore/signalr/messagepackhubprotocol.md Outdated Show resolved Hide resolved
aspnetcore/signalr/messagepackhubprotocol.md Outdated Show resolved Hide resolved
aspnetcore/signalr/messagepackhubprotocol.md Outdated Show resolved Hide resolved
aspnetcore/signalr/messagepackhubprotocol.md Outdated Show resolved Hide resolved
aspnetcore/signalr/messagepackhubprotocol.md Outdated Show resolved Hide resolved
aspnetcore/signalr/messagepackhubprotocol.md Outdated Show resolved Hide resolved
aspnetcore/signalr/messagepackhubprotocol.md Outdated Show resolved Hide resolved
aspnetcore/signalr/messagepackhubprotocol.md Outdated Show resolved Hide resolved
@analogrelay analogrelay requested a review from tdykstra February 27, 2019 19:41
Copy link
Contributor

@tdykstra tdykstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just one trivial suggestion.

aspnetcore/signalr/messagepackhubprotocol.md Outdated Show resolved Hide resolved
@analogrelay analogrelay merged commit 1fd1d3a into master Mar 5, 2019
@delete-merged-branch delete-merged-branch bot deleted the anurse/signalr-msgpack-quirks branch March 5, 2019 16:44
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

Successfully merging this pull request may close these issues.

5 participants