-
Notifications
You must be signed in to change notification settings - Fork 142
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
Bind routes to express-ws instance to make it easier to broadcast #122
base: master
Are you sure you want to change the base?
Conversation
Note: the only bit I’m not sure of in this is whether the |
This is what exactly I was looking for. |
@sberlinches If you need it right now, please feel free to use it from my fork until it’s in master. I’m using it in production with Site.js to power our funding/patronage pages at Small Technology Foundation. |
@aral Thanks! app.ws('/chat', function(ws, req) {
ws.on('message', message => {
this.getWss('/chat').clients.forEach(client => {
client.send(message)
})
})
});
app.ws('/another_route', function(ws, req) {
ws.on('message', message => {
this.getWss('/another_route').clients.forEach(client => {
client.send(message)
})
})
}); |
@aral My apologies for mentioning you again. It is just that I don't know if Github notifies to about an edited comment to the involved people. |
No worries and thanks for the report. I’ll investigate today and update when I have something. Update: Verified; now tracking here: https://source.ind.ie/site.js/app/issues/127 |
@sberlinches Hey Sergio, so I don’t know where I got it into my head that the I will revert the documentation change and update my pull request when I get a moment. In the meanwhile, here’s how you would get your example working with the current version. (Also note that in the code below the message is not sent to the client that it originated from either.) Hope this helps. app.ws('/chat', function(client, request) {
client.room = request.url.replace('/.websocket', '')
client.on('message', message => {
let count = 0
this.getWss().clients.forEach(c => {
if (c !== client && c.room === '/chat' && client.readyState === 1 /* WebSocket.OPEN */) {
c.send(message)
count++
}
})
console.log(`/chat broadcast to ${count} client${count === 1 ? '': 's'}.`)
})
})
app.ws('/another_route', function(client, request) {
client.room = request.url.replace('/.websocket', '')
client.on('message', message => {
let count = 0
this.getWss().clients.forEach(c => {
if (c !== client && c.room === '/another_route' && client.readyState === 1 /* WebSocket.OPEN */) {
c.send(message)
count++
}
})
console.log(`/another_route broadcast to ${count} client${count === 1 ? '': 's'}.`)
})
}) |
Right, I‘ve pushed two more commits. The first fixes the documentation regression I’d introduced by stating that the I’m happy with the state of this pull request and I don’t imagine any further additions to it. Please feel free to merge or not at your leisure. I’ve merged these changes into my fork, which is available from https://github.com/aral/express-ws/ |
OK, I lied, so I just pushed another commit that makes broadcast filtering (i.e., limiting broadcasts to just the clients on a certain route – for example to implement rooms in a chat app) quite elegant. With the latest commit, the example above becomes: const express = require('express');
const expressWs = require('express-ws')(express());
const app = expressWs.app;
function roomHandler(client, request) {
client.room = this.setRoom(request);
console.log(`New client connected to ${client.room}`);
client.on('message', (message) => {
const numberOfRecipients = this.broadcast(client, message);
console.log(`${client.room} message broadcast to ${numberOfRecipients} recipient${numberOfRecipients === 1 ? '' : 's'}.`);
});
}
app.ws('/room1', roomHandler);
app.ws('/room2', roomHandler);
app.listen(3000, () => {
console.log('\nChat server running on http://localhost:3000\n\nFor Room 1, connect to http://localhost:3000/room1\nFor Room 2, connect to http://localhost:3000/room2\n');
}); I’m going to integrate this into my master branch and into Site.js over the weekend. |
So when will this be released? |
@Ron-SSG Hey Ron, can’t speak for this project but I’ve been using it from my fork at https://github.com/aral/express-ws and it’s been part of Site.js for a while (https://sitejs.org). Hope that helps :) |
Hey @aral , started working with your fork, but it seems that when using it with a router i can't get access to Any ideas? |
@Ron-SSG Sorry, I don’t know why I’m not getting notifications properly here. I also don’t think I entirely understand the problem you’re running into. With Site.js, along with my fork, I simply add the websocket routes (at the end of the router chain): https://github.com/small-tech/site.js/blob/master/index.js#L780 ← does that help at all? (I’m going to assume not, most likely.) |
For those wondering what the current state of express-ws installed out of the box is: However, an easy work-around is just to add the clients to an array when they connect to a route, and remove them from the array when they disconnect. A 'send to all' function just loops through the array of the route. Not too hard. |
Use case
I have my routes defined in external files and they don’t have access to the original express-ws “instance” (the object returned by the
expressWs()
function).What this patch does
Binds WebSocket routes (
middlewares
) to the express-ws “instance” so that you can usethis
within a route to a reference to thegetWss()
function and the Expressapp
instance.Documents this as well as documenting the missing
route
parameter for thegetWss()
function.Example of use
From examples/chat.js:
Related issues
#7, #32, #80, #96