SocketCluster
Migrate to promises and async/await instead of callbacks
This should be done throughout all the code including socketcluster-client.
@jondubois are you going to do this?
someFunc(data,respond) {
return new Promise(async (resolve,reject) => {
// Do something
respond && respond(err,results)
return resolve()
})
}
or were you just going to remove callbacks all together?
@happilymarrieddad I think removing altogether is better or else it just adds more complexity than it's worth.
What are the plans here?
Node.js supports async / await since Node.js 8. And promises are a good foundation to use them.
For that it would be needed to drop support for Node.js <= 6.
6 is currently marked as Maintenance LTS, and will be EOL'd by April 2019. So that give us plenty of time
@JCMais I think it's OK to drop support for node <= 6 for new versions of SC.
I think we can start now. We may not need to do everything at once but we should make sure that the API remains consistent anyway.
Right now async/await is already partially supported for some things. With middleware, you can do this:
async function wait(timeout) {
return new Promise((resolve) => {
setTimeout(() => {
resolve();
}, timeout);
});
}
scServer.addMiddleware(scServer.MIDDLEWARE_HANDSHAKE_SC, async (req) => {
// Delay handshake by 5 seconds.
await wait(5000);
});
or
scServer.addMiddleware(scServer.MIDDLEWARE_HANDSHAKE_SC, async (req) => {
// Block handshake with error.
throw new Error('Handshake failure');
});
about your example, there is one thing that is missing on middlewares, you cannot have something similar to next(true), since throw true creates an error object with true.
@JCMais good point. I also just realized that yesterday and started making changes in socketcluster-server. What do you think about doing something like this:
async () => {
let err = new Error('Silent middleware error');
err.silent = true;
throw err;
};
this would also work since this thrown value is not logged anyway:
throw {silent: true};
For middlewares like MIDDLEWARE_HANDSHAKE_SC, which allow you to close the socket with a custom status code, we could do:
async () => {
let err = new Error('Handshake failed with custom code');
err.statusCode = 4444;
throw err;
};
Do you have alternative approaches to suggest?
I like that, socketcluster could also export two custom error objects to make those scenarios easier. So clients would just:
throw new StopSilentlyError()
or
throw new HandshakeError(4444)
Just an idea, the names are probably not that great.
@JCMais sounds good. These could be added to the shared sc-errors module.
For the client, I'm thinking about changing the API to something like this (feedback and suggestions are welcome):
// Consume response data from emitted event
try {
let adminUsers = await socket.emit('getUsers', {type: 'admin'});
// ...
} catch (err) {
// Failed to fetch users.
}
// Consume event data
async function consumeErrorEvent() {
for await (let error of socket.event('error')) {
// ...
}
}
async function consumeConnectEvent() {
for await (let connectStatus of socket.event('connect')) {
// ...
}
}
async function consumeCustomEvent() {
for await (let data of socket.event('customEvent')) {
// ...
}
}
// Consume channel data
async function consumeFooChannel() {
for await (let data of socket.channel('foo')) {
// ...
}
}
On the server side, the worker.exchange object should also follow a similar pattern for consuming channels.
@JCMais the channel object returned from socket.channel('foo') would be an iterable - Implemented as an async generator so you can chain them together in many ways. For example:
// A filtered stream based on the foo channel stream.
// It only contains messages published to the 'foo' channel which start
// with the letter 'A'.
async function* filteredFooGenerator() {
// socket.channel('foo') is an async generator/iterable exposed by
// the socketcluster-client API.
for await (let data of socket.channel('foo')) {
// This loop will iterate once for each message published to the 'foo' channel.
if (data.indexOf('A') === 0) {
yield data;
}
}
}
async function consumeFilteredFooData() {
let filteredFooStream = filteredFooGenerator();
for await (let data of filteredFooStream) {
// Each time a message is published to 'foo' channel which starts with the
// letter 'A',
// this loop will execute an iteration.
// ...
// Do something with the message; render it to a VueJS or React template, log it, ...
}
}
consumeFilteredFooData();
This is based on this approach: http://2ality.com/2018/04/async-iter-nodejs.html
I also wrote an article about how this could be implemented: https://hackernoon.com/using-async-generators-for-data-streams-f2cd2a1f02b3
It works similarly to RxJS Observables except it uses generators instead so the code should look a bit cleaner.
I think that implementation of the streaming functionality (e.g. using async generators) discussed above could wait a bit though (if the community likes that approach). Async generators are still very new and only implemented in Node.js 10 AFAIK.
Probably the first thing to do would be to switch to Promises for functions which expect a callback which triggers a single time. That shouldn't be controversial ;p
Also, we should consider separating regular events from events which expect a response (RPCs).
// Consume regular event
async function consumeCustomEvent() {
for await (let data of socket.event('customEvent')) {
// ...
}
}
// Consume RPC events
async function consumeCustomRPCEvent() {
for await (let req of socket.procedure('customRPC')) {
// ...
console.log(req.data); // req.data is the data passed to the RPC
if (...) {
req.error(error); // send back an error to the RPC caller
} else {
req.end(data); // send back data to the RPC caller
}
}
}
On the opposite side of the socket, the RPC can be invoked like this:
socket.invoke('customRPC', data)
.then((data) => {
// ...
})
.catch((err) => {
// ...
});
Events can be emitted like this:
// No Promise returned.
socket.emit('customEvent', data);
I'm thinking of also separating local events (which occur on the same side of the socket) from remote events which are transmitted by the socket on the opposite side of the connection.
// Handle local events
for await (let event of socket.listener('connect')) {
// ...
}
// Handle remote transmissions from counterparty socket.transmit('myReceiver', ...)
for await (let data of socket.receiver('myReceiver')) {
// ...
}
// Handle RPC requests from counterparty socket.invoke('myProcedure', ...)
for await (let request of socket.procedure('myProcedure')) {
// ...
}
// Handle channel data from counterparty socket.publish('myChannel', ...)
for await (let data of socket.channel('myChannel')) {
// ...
}
For each kind of stream above, it should be possible to listen only to the first event/message/request like this:
// For local events
let event = await socket.listener('connect').once();
// For remote transmissions
let data = await socket.receiver('myReceiver').once();
// For RPCs
let request = await socket.procedure('myProcedure').once();
// For channels
let data = await socket.channel('myChannel').once();
If anyone can think of better method names or a better pattern, feel free to suggest.
I think promisify is enought to archive that it don't matters if socketCluster it self uses callback or promise pattern as you can always convert with 1 till 3 lines of code from promise to callback and from callback to promise.
So This is simply waisting time if this would get applyed to this project it self. Its a Developer Desission if he wants to use a promise api or a callback api
Litle Maybe not working Pseudo code to make it clear
function usingCallback(callback) {
callback(err,result)
}
// to Promise
function promiseFromCallback(callback) {
return new Promise(function(resolve,reject){
usingCallback(function(err,result) {
if (result) { resolve(result) }
reject(err)
})
})
}
// from Promise
function callbackFromPromise(callback,promise) {
promise.then((result)=>callback(null, result)).catch((err)=>callback(err,null))
}
Hey @JCMais i know your junior thats why i will not rant about your rating but i hope you know that i am right i have over 30 years of expirence i am sure i am right :+1:
Migration is succsessfully completed. Since v15 SocketCluster extensively uses async/await and ComsumableStreams instead of callbacks. More info at https://socketcluster.io
