Conversation
bbe5f52
to
c2af960
Compare
5dab8bd
to
e853651
Compare
🆙📅 - rebased on latest dev |
e853651
to
a0fb5c1
Compare
client-ts/package.json
Outdated
"browserify": "^13.1.1", | ||
"del": "^2.2.2", | ||
"gulp": "^3.9.1", | ||
"gulp-typescript": "^3.1.3", | ||
"jasmine": "^2.5.2", | ||
"msgpack-lite": "^0.1.26", |
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 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.
Actually looking if we can use https://github.com/mcollina/msgpack5 which is not 0.x
import { IHubProtocol, MessageType, HubMessage, InvocationMessage, ResultMessage, CompletionMessage } from "./IHubProtocol"; | ||
import { BinaryMessageFormat } from "./Formatters" | ||
|
||
var msgpack = require("msgpack-lite"); |
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.
They added typings a day after I wrote this code. Switching to typings will make this ugliness go away.
"browserify": "^13.1.1", | ||
"del": "^2.2.2", | ||
"gulp": "^3.9.1", | ||
"gulp-typescript": "^3.1.3", | ||
"jasmine": "^2.5.2", | ||
"msgpack5": "^3.5.0", |
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.
@Eilon - non-0.x msgpack library for approval
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.
This package is approved for use here. I logged #696 for adding the third party notices file.
🆙📅 |
"browserify": "^13.1.1", | ||
"del": "^2.2.2", | ||
"gulp": "^3.9.1", | ||
"gulp-typescript": "^3.1.3", | ||
"jasmine": "^2.5.2", | ||
"msgpack5": "^3.5.0", |
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.
This package is approved for use here. I logged #696 for adding the third party notices file.
Before we would rely on error being null to detect whether to read results and we had an additional 'hasResult' field. Now all this information is codified in a field.
Makes it much easier to parse in JavaScript (also is more MsgPacky)
82eb84b
to
a3ab684
Compare
⌚ - still need a review. @Eilon approval is for usage of msgpack5 and not the PR in general. |
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.
Looks mostly mechanical 👍 . Can you write the spec next? That'll help me understand how we're using msgpack. It's really hard to look at tests with tons of hex 😄
Yes, spec update is coming. |
@@ -137,32 +148,41 @@ private void WriteMessageCore(HubMessage message, Stream output) | |||
|
|||
private static void WriteInvocationMessage(InvocationMessage invocationMessage, Packer packer, Stream output) | |||
{ | |||
packer.PackArrayHeader(5); |
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.
Note: it is not plugged in yet - #677 required to make it work end-to-end)