Error-first callbacks

[raine]

Hey. For instance the sendRequest method can't be promisified with the common tools because it's not returning the error first in the callback. Even if error wasn't possible due to the websocket architecture of listening to messages in the socket and picking the right callback based on seq, I think the first argument to the callback should be null.

Thoughts?

[liamcottle]

I'm not too sure what you mean here, could you elaborate with some examples?

[raine]

In node.js, it's idiomatic to pass an error, if any, as the first argument in callback functions to indicate if a call resulted in an error.

Promisifying functions such as util.promisify rely on this behavior.

In the case here, callbacks are called with the message as first argument: https://github.com/liamcottle/rustplus.js/blob/d3628d7a230648297e5c0425763ef127279f9469/rustplus.js#L80

Hence, if you try promisifying sendRequest or other methods, you'll see that successful response is registered as an error instead.

  const util = require('util')
  const rustplus = new RustPlus(...)

  const sendRequestAsync = util.promisify(rustplus.sendRequest.bind(rustplus))

  rustplus.on('connected', () => {
    sendRequestAsync({
      getInfo: {}
    })
      .then((res) => {
        console.log('result', res)
      })
      .catch((err) => {
        console.log('error', err) // logs the appinfo object
      })
  })

[liamcottle]

Ahh, I see what you mean now.

If I were to change the callback signature for sendRequest from (message) to (error, message), it would probably make sense for the sendRequest method to map all AppError responses to error and all others to message.

So it would end up being something like this:

rustplus.on('connected', () => {
    sendRequestAsync({
        getInfo: {}
    }).then((result) => {
        console.log('result', result); // this would be an AppResponse instance
    }).catch((error) => {
        console.log('error', error); // this would be an AppError instance
    });
});

The issue I have with doing this, is that in the case of catching it as an error, you would only be getting the AppError object, which means you lose access to the seq that's available in the parent AppResponse. Of course that's not too important, but in the future there could be other values that are needed or available.

To get around that, we could have it return the AppResponse, but then in that case, we now have then and catch both providing an AppResponse and we have to double handle.

Maybe there's a better way to get promises working with this. But then again, I'm unsure if it should be in the core of this library or not, because you may not ever get a result from the server for your request, which means the promise would never be fulfilled, and if you were to await the promise for some reason, it would just hang.

Is there a specific reason you're trying to turn sendRequest into sendRequestAsync ? As far as I'm aware, all requests would be synchronous at the websocket level anyway, since packets have to be fully sent before another can be sent.

Alternatively, you could wrap the response from the callback into your own promise if you're trying to pass it through processing pipes etc.

What are you thoughts on it?

[raine]

Maybe there's a better way to get promises working with this. But then again, I'm unsure if it should be in the core of this library or not, because you may not ever get a result from the server for your request, which means the promise would never be fulfilled, and if you were to await the promise for some reason, it would just hang.

I think it would ideal for the library to handle the timeout scenario by throwing a timeout error.

Is there a specific reason you're trying to turn sendRequest into sendRequestAsync ? As far as I'm aware, all requests would be synchronous at the websocket level anyway, since packets have to be fully sent before another can be sent.

I'm not sure what you mean, a promise API is nicer to work with.

Here's what I've ended up with for now, subject to change as I make progress: https://github.com/raine/ruoste_bot/blob/master/src/rustplus/rustplus-socket.ts#L93-L102

[liamcottle]

Cool, yeah handling timeouts in the library makes sense.

I've implemented sendRequestAsync in v2.1.1 and have added example 6_async_requests.js

• AppResponse is passed to resolve.
• AppError is passed to reject.
• Error('timeout') is passed to reject when the timeout is reached.
• You can set a custom timeout in milliseconds as the second parameter to sendRequestAsync.

Let me know what you think :)

[raine]

Thank you, it's great. Started using it right away.

I added a small wrapper around it still to throw a more readable error in case it's called before .connect(). This might happen if my bot is missing some rustplus configuration and hence not connecting.

ERROR: Cannot read property 'fromObject' of undefined
    TypeError: Cannot read property 'fromObject' of undefined
        at RustPlus.sendRequest (node_modules/@liamcottle/rustplus.js/rustplus.js:138:39)

^ You get that if you call sendRequest() before .connect()