Error handling in Auth phase

[keesschollaart81]

I'm using this library as part of the Home Assistant VS Code extension, I use ws for the WebSocket transport. By default, when setting up a WebSocket via ws, you cannot connect over SSL (WSS) when the certificate is not valid.

In the context of the HA VS Code Extension, I would like to allow connections over an insecure transport by providing ClientOptions (which has a rejectUnauthorized property) to the websocket constructor. [related issue].

Next to that, I added some logging during the auth phase so that if errors like this occur, it's easier to see what's going on.

[balloob]

https://developer.mozilla.org/en-US/docs/Web/API/WebSocket#Constructor

There is no second option argument.

[keesschollaart81]

Correct for the browsers WebSocket implementation. In the context of Node, using the ws based WebSocket, it has:

https://github.com/websockets/ws/blob/master/doc/ws.md#new-websocketaddress-protocols-options

The rejectUnauthorized is not directly specified for the options object because it's part of https.request().

So it depends on which WebSocket implementation you use!? (hence why I made it optional?)

[balloob]

So I have opened #91 so that this PR can attach the error that caused the event to the raised error.

However, will this really work ? Because we will not be able to determine if an error is critical or that we should keep retrying. And so for this code to really work properly, won't consumers of this lib in NodeJS just fork getSocket anyway? And if they fork that anyway, there is no reason to merge #91 and we should remove passing a websocket as a constructor again.

It would almost make more sense to offer a standalone getSocketForNodeJS function that can be used instead of trying to make a one size fits all approach.

CC @zachowj - how do you currently deal with WebSocket errors?

[zachowj]

And so for this code to really work properly, won't consumers of this lib in NodeJS just fork getSocket anyway

Since I need to still support api_password and deal with a wierd hass.io proxy issue. I just pass in a custom createSocket and handle everything in there.

[keesschollaart81]

With #90 you basically introduced a factory method for the WebSocket implementation. This allows me to add event listeners myself. So even if you then swallow the errors in this library, I could subscribe to errors myself and deal with that, somehow...

[balloob]

Well that doesn't seem like a nice solution… to do it proper you might want to just include your own createSocket too…

[keesschollaart81]

Yes! Agree, let go do that then! Close this PR and move forward in yours?

[balloob]

Well if you are going to use your own createSocket, I will probably not move forward with the better error handling and also revert the custom websocket. We should update the readme to have people fork the createSocket.

[keesschollaart81]

So looking at @zachowj's implementation of the WebSocket, I would go and pretty much copy everything. Then it's also very similar to the implementation currently in this library. It would be great is we could somehow combine all these needs / logic. If feels it should be possible somehow and it would make changes in the websocket-integration easier later, having it in 1 place (even without bringing in the whole ws package).

[balloob]

Well @zachowj has events emitting, alternative url based on Hass.io, API password support, WS options injection.

I think that it's very difficult to generalize, without also blowing up the scope of the package. Sure, we could create a Node.js specific createSocket function that Node.js people can import and add to the options, but what if that doesn't satisfy all requirements again ?

createCreateSocket(ws, rejectUnauthorizedCerts) => createSocketFunc

[balloob]

I am going to close this. I suggest that if we want to support Node properly, someone submits a PR with a generic createSocket function to use. It should however not be bundled in the main bundle but be published as a standalone file, so that it doesn't bother browsers.