/ng2-signalr

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

Feature - Add option to select transport type #53

Merged
merged 3 commits into from May 21, 2017

Conversation

Reviewers

@HNeukermans HNeukermans

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@Saaka @HNeukermans
@Saaka
Contributor

Saaka commented May 16, 2017

Add option to select transport type: specify one transport or ordered fallback list.
@HNeukermans

HNeukermans reviewed May 17, 2017
View changes

Awesome PR. @Saaka.
Valueable feature you have added there.
I've carefully reviewed you code.
If you can fix the remarks I've made I will be very happy to merge it in :-)

src/services/signalr.ts Outdated
@@ -25,13 +25,15 @@ export class SignalR {
let configuration = this.merge(options ? options : {});
try {
let serialized = JSON.stringify(configuration.qs);
let serializedQs = JSON.stringify(configuration.qs);
let serializedTransport = JSON.stringify(configuration.transport);

This comment has been minimized.

Sign in to view
Copy link
@HNeukermans

HNeukermans May 17, 2017

Owner

no need to serialize since it is just a string

This comment has been minimized.

Sign in to view
Copy link
@Saaka

Saaka May 17, 2017

Contributor

It is serialized, because it is string or array of strings.
src/services/signalr.ts
if (configuration.logging) {
console.log(`Connecting with...`);
console.log(`configuration:[url: '${configuration.url}'] ...`);
console.log(`configuration:[hubName: '${configuration.hubName}'] ...`);
console.log(`configuration:[qs: '${serialized}'] ...`);
console.log(`configuration:[qs: '${serializedQs}'] ...`);
console.log(`configuration:[transport: '${serializedTransport}'] ...`);

This comment has been minimized.

Sign in to view
Copy link
@HNeukermans

HNeukermans May 17, 2017

Owner

you can change this to console.log(configuration:[transport: '${configuration.transport}'] ...); ?
src/services/signalr.configuration.ts Outdated
@@ -19,13 +19,17 @@ export class SignalRConfiguration {
/** Allows withCredentials. This flag can be used to suppport CORS */
public withCredentials: boolean;
/**Allows you to specify transport. You can specify a fallback order if you wan't to try specific transports in order. */
public transport?: any;

This comment has been minimized.

Sign in to view
Copy link
@HNeukermans

HNeukermans May 17, 2017

Owner

Is this of type string? array of strings? Can you explain the possible values you are expecting

This comment has been minimized.

Sign in to view
Copy link
@Saaka

Saaka May 17, 2017

Contributor

This is either a string or array of strings. I can change it to array of TransportConnection objects, and change it to string or array of strings before passing it to connect function. This way user wont be able to pass invalid parameters.

This comment has been minimized.

Sign in to view
Copy link
@HNeukermans

HNeukermans May 17, 2017 •
edited

Owner

@Saaka
great idea. great minds think alike :-)
make it ConnectionTransport[] then , agree?
src/services/connection/connection.transport.ts
@@ -0,0 +1,6 @@
export class ConnectionTransport {

This comment has been minimized.

Sign in to view
Copy link
@HNeukermans

HNeukermans May 17, 2017

Owner

Could you design your ConnectionTransport as a 'value' object. You can use ConnectionStatus/ConnectionStatuses as a example. I use this design because signalr connection status is basically a string that has only limited range of values. This design prevents the api user from filling in something random

This comment has been minimized.

Sign in to view
Copy link
@HNeukermans

HNeukermans May 17, 2017

Owner

Also make sure to test your ConnectionTransports & Connectiontransport object. You can use
connection.statuses.spec.ts & connection.status.spec.ts as an example.
tests/services/signalr.configuration.spec.ts Outdated
@@ -8,6 +8,7 @@ describe('SignalRConfiguration', () => {
expect(configuration.hubName).toBe(null);
expect(configuration.qs).toBe(null);
expect(configuration.url).toBe(null);
expect(configuration.transport).toBe(null, 'transport should be null');

This comment has been minimized.

Sign in to view
Copy link
@HNeukermans

HNeukermans May 17, 2017

Owner

nice

This comment has been minimized.

Sign in to view
Copy link
@HNeukermans

HNeukermans May 17, 2017

Owner

then make this an empty array instead of null
src/services/connection/connection.options.ts Outdated
@@ -14,4 +14,7 @@ export interface IConnectionOptions {
/** Allows withCredentials */
withCredentials?: boolean;
/**Allows you to specify transport. You can specify a fallback order if you wan't to try specific transports in order. */

This comment has been minimized.

Sign in to view
Copy link
@HNeukermans

HNeukermans May 17, 2017

Owner

to keep linter happy please a space:
instead of /**XXXX do /** XXX
@HNeukermans

HNeukermans reviewed May 17, 2017
View changes

Nice works @Saaka,

Keep in mind to create these files:
TransportConnection.ts equivalent to ConnectionStatus
TransportConnections.ts equivalent to ConnectionStatuses
TransportConnection.spec.ts equivalent to ConnectionStatus.spec.ts
TransportConnections.spec.ts equivalent to ConnectionStatuses.spec.ts

Let me know if thing are unclear.

src/services/signalr.configuration.ts Outdated
constructor() {
this.hubName = null;
this.logging = false;
this.qs = null;
this.url = null;
this.jsonp = false;
this.withCredentials = false;
this.transport = null;

This comment has been minimized.

Sign in to view
Copy link
@HNeukermans

HNeukermans May 17, 2017

Owner

this.transport = [];
README.md Outdated
// <= v1.0.9
const config = new SignalRConfiguration();
config.hubName = 'Ng2SignalRHub'; //default
config.qs = { user: 'donald' };
config.url = 'http://ng2-signalr-backend.azurewebsites.net/';
// Specify one Transport: config.transport = ConnectionTransport.LONG_POLLING; or fallback options with order like below.
config.transport = [ConnectionTransport.WEB_SOCKETS, ConnectionTransport.LONG_POLLING];

This comment has been minimized.

Sign in to view
Copy link
@HNeukermans

HNeukermans May 17, 2017

Owner

can you makes this config.transport = [ConnectionTransports.webSockets, ConnectionTransports.longPolling]
src/services/signalr.ts Outdated
@@ -65,6 +65,17 @@ export class SignalR {
return $promise;
}
private getConnectionStartParams(withCredentials: boolean, jsonp: boolean, transport: any) : any {

This comment has been minimized.

Sign in to view
Copy link
@HNeukermans

HNeukermans May 17, 2017

Owner

let jTransports = convertTransports(configuration.transports);

jConnection.start({ withCredentials: configuration.withCredentials, jsonp: configuration.jsonp, transport: jTransports })

convertTransports(Transports ) {
return transports.map((t) => t.name); ...
}
@Saaka

This comment has been minimized.

Sign in to view
Contributor

Saaka commented May 17, 2017 •
edited

I have made transport as follows:
public transport: ConnectionTransport | ConnectionTransport[];
I did not like it to be array, because even if you want to specify one, you still need array. I also made change to the default value based on the jquery lib. Now it defaults to "auto", same as in base library.
@HNeukermans

This comment has been minimized.

Sign in to view
Owner

HNeukermans commented May 18, 2017 •
edited

allright @Saaka,
you are a Pro !! :-)
I will do a git clone of your repo, run the unit test & do some E2E tests and then merge the PR, if everything is ok.

@HNeukermans HNeukermans merged commit 4c30691 into HNeukermans:master May 21, 2017
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment