-
Notifications
You must be signed in to change notification settings - Fork 6
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
Upgrade Sugar to Primus 8 #185
Conversation
The Primus client is downloaded from https://notifications-staging.zooniverse.org/primus.js. The Sugar client is copied from https://github.com/zooniverse/Sugar-Client/ and updated to Primus 8 connection options. |
I've tested this with some projects on a staging branch of PFE. The client connects to Sugar and subscribes to project channels, as expected. |
After this merges, we can deprecate https://zooniverse/Sugar-Client. I think this was the only package that used it. |
9f08988
to
ee0dcd8
Compare
I've not been able to see any problems with this version of the client, after testing it on PFE staging. |
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.
PFE - confirmed on https://pr-6368.pfe-preview.zooniverse.org/projects/markb-panoptes/focus-testing-project/talk?env=staging that when user 1 loaded the project Talk and user 2 loaded the project Talk - user 2 saw "2 active participants" on load, and user 1 saw "2 active participants" on refresh (I think all as expected) ✅ .
FEM - see this FEM branch, installed the primus-8 branch, confirmed that when user 2 messaged user 1, user 1's Messages and Notifications in header increased as expected ✅ .
Should the test files from Sugar-Client be added to the panoptes-javascript-client?
this.primus = SugarClient.Primus.connect(SugarClient.host, { | ||
websockets: true, | ||
network: true, | ||
manual: true |
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.
The only difference between this and https://github.com/zooniverse/Sugar-Client/blob/master/client.js that I'm seeing is the deletion of ping: 10000
(link). Apologies if it has already been stated somewhere, but is that property no longer needed with Primus 8, and it was with Primus 6?
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.
Possibly related - https://github.com/primus/primus#protocol
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.
It’s been removed because the direction of the ping/pong protocol changed from client->server to server->client. There’s a list of client options in the Readme.
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.
So that option said: ping the server then wait 10s for a pong response. The client now sends pong back in response to pings from the server.
You can see the traffic if you watch the web socket in browser dev tools.
That’s an excellent idea. |
- add the Sugar and Primus 8 client libraries to `lib/SugarClient`. - update `lib/sugar.js` to use the local libraries. - remove `sugar-client`.
ee0dcd8
to
8af5939
Compare
- add the Sugar and Primus 8 client libraries to `lib/SugarClient`. - update `lib/sugar.js` to use the local libraries. - remove `sugar-client`.
- add the Sugar and Primus 8 client libraries to `lib/SugarClient`. - update `lib/sugar.js` to use the local libraries. - remove `sugar-client`.
lib/SugarClient
.lib/sugar.js
to use the local libraries.sugar-client
.