Removed double error throw for subscribeFail

[mattkrick]

Currently, if a sub fails, it calls the subscribeFail callback, which is good. However, It additionally emits an error, which is bad. No reason to emit the same error across 2 channels! Additionally, the error it emits is just a string (bad), whereas all other errors are error objects (good).

Pretty sure this is a bug, so I closed it up.

[jondubois]

At the moment, the 'error' event is designed to emit any error triggered on the client. The 'error' event is mostly there for logging purposes - It allows you to be notified if anything goes wrong on the client socket - But it's too generic (too many different kinds of errors) to allow you to actually do anything other than logging. It's there to help the developer capture errors for debugging purposes.

The 'subscribeFail' event is more specific and it's intended to be used for actually handling the error - For example, in response to this event, you may want to retry subscribing again or show the user an error dialog...

What you're saying makes sense (maybe we should have two distinct error types with no overlap - So in effect, we can consider 'subscribeFail' to be a low-importance error which the user can choose to capture/log independently of the main 'error' event...), but we'll have to change the error-handling behaviour for not just the '#subscribe' case, but also the '#publish' case and maybe others too.

The publish method accepts a callback(err, ...) - If it fails, we get the error inside the callback, so maybe it also shouldn't get emitted as a separate 'error' event either? Or maybe we should instead emit a 'publishFail' event instead of 'error'.

I don't mind moving in that direction but we need to come up with a more general philosophy first about handling these 'special errors'.

Also, note that the error is a string in these cases because the error was sent back directly from the server (so we can't really show a stack trace).

[mattkrick]

Interesting, I came to the same conclusion, but didn't see a clean way to handle it without a breaking API change. For my use case, I only care about errors that add value to the user. For example...

A connectionError, including connecting, reconnecting, and acks/heartbeats. This is useful to the user because we can put a "please hold, reconnecting" modal up when it's thrown. Since it comes from the client, it'll be an Error object.

Then we've got authenticationError which is an invalid JWT. This is useful because we can send them back to the login page & put an error at the top of it (eg "your 3 hour trial has ended"). I think it's kinda weird to send this as a string in the connect event, since it's possible that event is emitted before authorization finishes. Instead, we can send an error object & token to #authenticate. If the error is non-null, we rehydrate it back into an Error object, then send that object out via the authenticateError event.

Then we've got all the middleware errors, which is an authorizationError (this name is similar to the one above, maybe call the authentication error a jwtError or tokenError?). This is useful for anything permission based (eg "This is for Platinum members only!"). Note: currently, middleware errors can also be authentication errors. we need to change this (see SocketCluster/socketcluster-server#1)

I'd argue that sending the stack trace to the client is at best useless & at worst dangerous (exposes server internals). Instead, the server can send an object with only type & message & in the #authenticate handler, we rehydrate it into an Error. This way, devs can write a handler that parses on error type & not the string. They are also guaranteed to receive an Error object & don't have to check if it's a string or object.

Finally, we can keep the error around if useful, but make it explicit that it's for debugging purposed only.

[jondubois]

Mentioned this as part of v4 RFC SocketCluster/socketcluster#137

[jondubois]

Feedback regarding this PR:

Here we are passing the err object directly - Shouldn't it be callback(token)(err)? https://github.com/mattkrick/socketcluster-client/blob/waitForAuth-multiplex/lib/sctransport.js#L110

I'm not a big fan of functions which return a function all inline like this https://github.com/mattkrick/socketcluster-client/blob/waitForAuth-multiplex/lib/sctransport.js#L90-L91 - It's a bit hard to read and makes it prone to bugs like the one above.

eventObject.event !== "#subscribe" - We should fix this properly in SC v4 with improved error-handling for middleware-generated errors https://github.com/mattkrick/socketcluster-client/blob/waitForAuth-multiplex/lib/sctransport.js#L177 - It feels wrong to fix it for the subscribe case but not for publish and other middleware errors - I think we should fix them all at once since consistency is more important in this case.

[jondubois]

I think once you fix the things mentioned above, I can merge most of this in.

That said, I won't merge the code related to the auth status just yet. I need to think about it more - I would like to consider other options before settling on this one. I think a problem with this one is that it assumes that the time on the server (for the expiry) is the same as the time on the client which isn't always the case (E.g. if the client is in a different country/timezone than the server).

You could add an extra field to the token to record the expiry in terms of seconds until it expires (instead of an absolute time) but I'm still not completely sure this would be the best approach.

One thought I want to consider more (not sure if this is a good idea yet though) was to let the server be responsible for telling the client that the token has expired or is no longer valid. I think this is a tricky problem which should be investigated further.

[mattkrick]

Sounds reasonable, i'll respond to waitForAuth stuff in that PR.

[jondubois]

@mattkrick Can I close this PR? This clashes with the current error-handling philosophy where the 'error' event is just for logging purposes. I'd like to get the whole error handling approach fixed in a single release v4.

[mattkrick]

Yep!