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

enable socketio clients to communicate with client using only the protocols set in the config #39

Merged
merged 4 commits into from
Oct 21, 2016

Conversation

shriramshankar
Copy link
Contributor

No description provided.

@shriramshankar shriramshankar changed the title force socketio to use only polling Do Not Merge force socketio to use only polling Oct 12, 2016
@meynet-salesforce meynet-salesforce temporarily deployed to refocus-pr-39 October 12, 2016 17:59 Inactive
@meynet-salesforce meynet-salesforce temporarily deployed to refocus-pr-39 October 12, 2016 18:03 Inactive
@meynet-salesforce meynet-salesforce temporarily deployed to refocus-pr-39 October 12, 2016 18:12 Inactive
@shriramshankar shriramshankar temporarily deployed to refocus-pr-39 October 12, 2016 23:42 Inactive
@shriramshankar shriramshankar temporarily deployed to refocus-pr-39 October 16, 2016 20:37 Inactive
@shriramshankar shriramshankar temporarily deployed to refocus-pr-39 October 16, 2016 21:13 Inactive
@shriramshankar shriramshankar temporarily deployed to refocus-pr-39 October 16, 2016 21:20 Inactive
@shriramshankar shriramshankar temporarily deployed to refocus-pr-39 October 19, 2016 23:57 Inactive
@shriramshankar shriramshankar temporarily deployed to refocus-pr-39 October 20, 2016 06:41 Inactive
@shriramshankar shriramshankar temporarily deployed to refocus-pr-39 October 20, 2016 18:34 Inactive
@shriramshankar shriramshankar temporarily deployed to refocus-pr-39 October 20, 2016 19:01 Inactive
@shriramshankar shriramshankar temporarily deployed to refocus-pr-39 October 21, 2016 06:16 Inactive
@shriramshankar shriramshankar changed the title Do Not Merge force socketio to use only polling enable socketio clients to communicate with client using only the protocols set in the config Oct 21, 2016
@shriramshankar shriramshankar temporarily deployed to refocus-pr-39 October 21, 2016 06:19 Inactive
@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 83.37% when pulling 94126a9 on socketio-polling into a402180 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 83.37% when pulling 94126a9 on socketio-polling into a402180 on master.

@shriramshankar shriramshankar temporarily deployed to refocus-pr-39 October 21, 2016 06:54 Inactive
@shriramshankar
Copy link
Contributor Author

All the changes are here. Will do the any other requested changes after my training tomorrow.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 83.37% when pulling 1e03469 on socketio-polling into a402180 on master.

@@ -26,10 +26,14 @@ const DEFAULT_THROTTLE_MILLISECS = 4000;
const realtimeEventThrottleMilliseconds =
pe.realtimeEventThrottleMilliseconds || DEFAULT_THROTTLE_MILLISECS;

const socketIOtransportProtocol = pe.SOCKETIO_TRANSPORT_PROTOCOL || 'undefined';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why using string 'undefined' here? It seems like it has potential for misunderstanding given javascript's "undefined" reserved word. I would rather see something like this:

const socketIOtransportProtocol = pe.SOCKETIO_TRANSPORT_PROTOCOL || null;

or

const socketIOtransportProtocol = pe.SOCKETIO_TRANSPORT_PROTOCOL || '';

... and then in app.js line 105, just check for if (clientProtocol) {

const options = {};
const clientProtocol = transProtocol;
if (clientProtocol !== 'undefined') {
options.transports = clientProtocol.replace(/\s*,\s*/g, ',').split(',');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment about this regex and the format the socket.io options.transports requires?

@shriramshankar shriramshankar temporarily deployed to refocus-pr-39 October 21, 2016 21:48 Inactive
@shriramshankar
Copy link
Contributor Author

pr updated
@theswamis added the option to force client to use websocket using the query param.
@iamigo used '' instead of undefined.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 83.37% when pulling fef5e50 on socketio-polling into a402180 on master.

@shriramshankar
Copy link
Contributor Author

shriramshankar commented Oct 21, 2016

?protocol=websocket is the query param to force the client to communicate over websocket.

@shriramshankar
Copy link
Contributor Author

shriramshankar commented Oct 21, 2016

@pallavi2209 @iamigo web concurrency also seems to be working. Can you check if it set the config var right ? I set WEB_CONCURRENCY = 2.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.007%) to 83.37% when pulling 29ee2fa on socketio-polling into e2d4e2c on master.

@pallavi2209
Copy link
Contributor

@shriramshankar Thats great! The config var looks right to me.

@iamigo iamigo merged commit 4aa36ab into master Oct 21, 2016
@iamigo iamigo deleted the socketio-polling branch October 21, 2016 23:08
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