ocilo / skype-http

Unofficial Skype API for Node.js via HTTP
https://ocilo.github.io/skype-http
MIT License
51 stars 24 forks source link

Shouldn't return a rejection as nothing can listen for that, however … #33

Closed mitchcapper closed 7 years ago

mitchcapper commented 7 years ago

…we do have an error handler for exceptions

demurgos commented 7 years ago

Since we are in an async function, returning a rejected Promise is the same as throwing an error.

I know that throwing an error prevented some optimizations, that's why I did not try to replace every reject by a throw. Anyway, I don't think that the goal of the library is performance: most of the time is spent waiting for HTTP requests. If you feel that it would make it clearer, we can merge it.

mitchcapper commented 7 years ago

Sorry I should have been more clear. Normally yes, returning a reject is fine however this function is not called with an await, and not called by the user. It cannot be caught as setInterval is calling it: https://github.com/ocilo/skype-http/blob/master/src/lib/polling/messages-poller.ts#L165

mitchcapper commented 7 years ago

You could change listen to be async and not return while running (and then rather than setInterval just use a while loop with a await delay). Then you could properly TC on the listen loop

demurgos commented 7 years ago

I see. Even if it is not called with await, the function is still async so the throw will result in a rejected promise. The issue is that the caller does not resolve this promise.

Since this code is event-based (as opposed to simple request-responses), I think that a better solution would be to keep listen synchronous but make it resolve the promise and use this.emit("error", err) if an error occurs.

demurgos commented 7 years ago

Superseded by #34