Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Adapting TS-client to work with non-level-2 xhr #744

Merged
merged 1 commit into from
Aug 24, 2017
Merged

Conversation

moozzyk
Copy link
Contributor

@moozzyk moozzyk commented Aug 21, 2017

Also exporting MessagePackHubProtocol

@@ -168,6 +168,12 @@ export class LongPollingTransport implements ITransport {
connect(url: string, requestedTransferMode: TransferMode): Promise<TransferMode> {
this.url = url;
this.shouldPoll = true;

let actualTransferMode =
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 you meant to use this

Copy link
Contributor Author

@moozzyk moozzyk Aug 22, 2017

Choose a reason for hiding this comment

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

I will remove this code.

This is a leftover of my failed attempt to send MessagePack over non-level-2 xhr. This won't work because the server assumes it knows transfer modes a transport supports. I opened #742 for this.

if (pollXhr.response) {
console.log(`(LongPolling transport) data received: ${pollXhr.response}`);
this.onDataReceived(pollXhr.response);

Copy link
Member

Choose a reason for hiding this comment

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

nit: newline

@moozzyk moozzyk force-pushed the pawelka/ts-node branch 2 times, most recently from 509f304 to 17f13c7 Compare August 23, 2017 00:16
@moozzyk
Copy link
Contributor Author

moozzyk commented Aug 23, 2017

🆙📅

Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

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

From my limited knowledge looks good

@davidfowl
Copy link
Member

Looks clean but I still don't want to require polyfills to work with node. This is fine for the alpha


if (requestedTransferMode === TransferMode.Binary && (typeof new XMLHttpRequest().responseType !== "string")) {
// This will work if we fix: https://github.com/aspnet/SignalR/issues/742
throw new Error("Binary protocols over XmlHttpRequest not implementing advanced features not supported.");
Copy link

Choose a reason for hiding this comment

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

nit: Binary protocols over XmlHttpRequest not implementing advanced features are not supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure.

@moozzyk
Copy link
Contributor Author

moozzyk commented Aug 24, 2017

We have an issue for not using polyfills on node. This is post-alpha.

Note that this change is a bit more general - it makes signalr client work on browsers that don't support level 2 features (looks like only opera mini falls in this category though)

@davidfowl
Copy link
Member

Note that this change is a bit more general - it makes signalr client work on browsers that don't support level 2 features (looks like only opera mini falls in this category though)

Opera mini 🤣

@moozzyk moozzyk merged commit cddc5f8 into dev Aug 24, 2017
@moozzyk moozzyk deleted the pawelka/ts-node branch September 15, 2017 19:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants