Integrating MsgPack support in TS client #699
Conversation
| export interface IHubProtocol { | ||
| name(): string; | ||
| type(): ProtocolType; |
There was a problem hiding this comment.
You can do real properties in typescript.
There was a problem hiding this comment.
I did not want them to be settable.
There was a problem hiding this comment.
(from JS that is...)
There was a problem hiding this comment.
Cant you do get only properties?
There was a problem hiding this comment.
You can do readonly properties but they are only readonly in TypeScript but not in Javascript.
There was a problem hiding this comment.
on the other hand you could replace the entire function in JS so meh - can change to a property (same for name())
| send(data: any): Promise<void>; | ||
| stop(): void; | ||
| onDataReceived: DataReceived; | ||
| onClosed: TransportClosed; | ||
| transferMode(): TransferMode; |
There was a problem hiding this comment.
I am thinking about removing this and just returning Promise from connect. Same for connection.
| @@ -112,10 +112,10 @@ describe("Connection", () => { | |||
| } | |||
| } as IHttpConnectionOptions; | |||
|
|
|||
| var connection = new HttpConnection("http://tempuri.org", options); | |||
| let connection = new HttpConnection("http://tempuri.org", options); | |||
There was a problem hiding this comment.
const connection instead? (here and elsewhere)
There was a problem hiding this comment.
I don't think so - this is not an object literal so I don't think it makes any difference. Conceptually this object is not a const since it exposes onClosed and onDataReceived properties which are settable.
There was a problem hiding this comment.
I don't mind either way, but const only makes the variable constant (you can't assign something else to the name), not what the variable is referencing. (See readonly for that.)
There was a problem hiding this comment.
I am leaving it as it is then. If we decide to change it to const we would probably have to change almost all variables which is out of scope for this PR. I was just annoyed by mixing let and var without any reason.
| @@ -32,18 +32,18 @@ export class HttpConnection implements IConnection { | |||
| this.options = options; | |||
| } | |||
|
|
|||
| async start(): Promise<void> { | |||
| async start(requestedTransferMode: TransferMode): Promise<void> { | |||
There was a problem hiding this comment.
What's the alternative? Features? Adding Features felt to heavy for this. We can do this but it would come separately.
| @@ -120,6 +120,14 @@ public class HubEndPoint<THub> : IInvocationBinder where THub : Hub | |||
| ? (IDataEncoder)Base64Encoder | |||
| : PassThroughEncoder; | |||
|
|
|||
| var transferMode = connection.Features.Get<ITransferModeFeature>() ?? | |||
There was a problem hiding this comment.
I now understand how this sneaked in - #711
|
🆙📅 - Added features to IConnection |
| readonly type: ProtocolType; | ||
|
|
||
| parseMessages(input: any): HubMessage[] { | ||
| // atob/btoa are browsers APIs but they can be polyfilled. If this becomes problematic we can use |
There was a problem hiding this comment.
File a bug for that. I don't want too many untracked browser deps
There was a problem hiding this comment.
Are we going to do anything about this though? I did try base64-js package and reverted the change since it added 2.2 KB to the library (minified) which is wasteful on browsers since the functionality is built-in. If you hit this on node you would need to polyfill. Unless the bug is supposed to be a doc bug which will be turned into a an article about dependencies a user needs to polyfill when running on node.
There was a problem hiding this comment.
I can think of a few things we might do here:
- Polyfill on nodejs - I hate this option
- Dependency injection
- Build a nodejs version with different sources
There was a problem hiding this comment.
...we could also not support binary over SSE on node. I think XmlHttpRequest and WebSockets are more important - once we decide about these the atob/btoa replacement will just follow the suit.
moozzyk
force-pushed
the
pawelka/msgpack-js-integrate
branch
from
0b4008d to
ed59d7f
Compare
No description provided.