-
-
Notifications
You must be signed in to change notification settings - Fork 44
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
#576 Drop dependencies to Primus and Primus plugins #1052
Conversation
6b1448f
to
36f51c9
Compare
2865f4b
to
0494fa2
Compare
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.
Initial review; it's end of day for me now, I will likely post further review comments later :)
20b2ab0
to
f875568
Compare
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.
@snowteamer this is a really fantastic PR you've sent in! Because of its size, it will take me a bit longer to review fully, however I've gone ahead and left some feedback for now so that you can work on it / respond to it while I continue to review.
5cb3cb6
to
77d2f41
Compare
bb9de23
to
640f6f1
Compare
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.
@snowteamer OK this is my final review I think. Only minor things remaining, posted comments, plus a request to use okTurtles.events
where possible unless there's a reason not to. You're already using it in some parts of the code, I'm not sure why we need to re-implement event emission, and if there isn't a strong reason to, let's not do that.
frontend/main.js
Outdated
sbp('okTurtles.data/set', PUBSUB_INSTANCE, createGIPubSubClient( | ||
pubsubURL, { | ||
// This option can be enabled since we are not doing auth via web sockets. | ||
reconnectOnTimeout: 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.
Here we lost the timeout
value of 3000, could that be added back in please (especially since the default appears to be 5000)?
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.
That was on purpose, 3000 is likely too short:
- We have seen that timing issues can cause the Travis build to fail.
- All web socket libraries that I have checked use a higher default timeout or do not seem to specify any at all:
- reconnecting-websocket: 4000
- Primus: 10000
- socket.io: 20000
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.
ok, it's still useful to specify it here though to remind us that's an option and what its current value is without diving into the implementation. To your first point, we can also extend it for travis, e.g. something like this:
timeout: process.env.NODE_ENV === 'development' ? 10000 : 5000 // extend for Travis
Unfortunately |
Ok, I replied with a suggestion on Slack |
0095345
to
2bb4851
Compare
e01cd71
to
1beb6a1
Compare
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.
Approved! Excellent work @snowteamer! 😄 🥇 🎉
Closes #576
Summary of changes:
primus-rooms
andprimus-responder
plugins in favor of a pubsub implementation using only thews
socket library in Node and the WebSocket API in the browser.'backend/server/broadcastEntry'
SBP selector.Additional information:
The core of this proposed pubsub framework consists of only two files:
~/backend/utils/pubsub.js
which exports acreateServer()
function.~/shared/pubsub.js
which exports acreateClient()
factory suitable to create either ordinary pubsub clients in the browser or server-side clients in Node.Such server-side clients can be useful e.g. in testing or to talk to other servers.
Currently, it comes with a set of so-called "default" event handlers, designed to handle only basic pubsub protocol stuff and
ENTRY
messages as required by the application.However this protocol could be extended by providing appropriate "custom" message handlers to the factories.
In progress:
Primus has automatic reconnection features which are not all implemented in this PR yet:
online
event occurs, including in case the web frontend was started in offline mode. It seems to be working now, but again tests are still missing.offline/online
events using ping/pong messages (https://github.com/primus/primus#heartbeats-and-latency).All these features are options so they can be disabled if we do not need them.
Again, testing these features might be tricky, so any help or advice will be much appreciated.