Don't expect objects on callback

[silverbucket]

When sockethub completes there isn't always an object to send back, as it didn't make sense to send back the same object that was sent. If the handler completes, you can expect it was successful unless there's an error returned.

However, that would have been pretty fast to catch, this took me a several hours to track down as all I was seeing was a disconnect (on the client side, not on the server side) followed by an instant second connection, both of which stayed alive until the end. In the end, this was the offending line:

https://github.com/67P/hyperchannel/blob/master/app/services/sockethub-xmpp.js#L143

const channelId = message.target.id.split('/')[0];

The target was not expanded because the completed join handler did not send back a copy of the same object that was sent. This should have simply thrown some exception, but something seems to be silencing the error, is this an ember thing? As a side effect, the (silenced?) exception seemed to effect the client-side socket.io connection while not effecting the server side. So the client was in some kind of 1 zombie 1 active socket connection state with no error on the console. Very weird.

I haven't had a look at the IRC side of things, so assume this might effect it too.

[raucao]

This is clearly a breaking change to the platform API, which should not happen in a PR that doesn't intend to introduce breaking changes. The docs still state that the callback gets an object as function argument, and I also remember very well that I found it a bit weird to be checking for the error property on that object, but that's exactly how we implemented this according to the API.

So this fix isn't really the ideal fix for the underlying problem, since any other client that implements the API as it was, and as it's still documented, will break with that change.

[raucao]

his should have simply thrown some exception, but something seems to be silencing the error, is this an ember thing?

No, there is nothing special that Ember does there. It's pretty much vanilla JS in that regard.

[silverbucket]

No, there is nothing special that Ember does there. It's pretty much vanilla JS in that regard.

Any idea what could be causing it? I was unable to find any reason for it. I don't think it has anything to do with Sockethub, as this doesn't happen anywhere else. It seems specific to the hyperchannel app environment, something about it is suppressing (e.g. maybe trying to handle in another context) exceptions.

Normally we'd have seen the exception, and it would have been clear where the issue was:

const channelId = message.target.id.split('/')[0];

VM152:1 Uncaught ReferenceError: message is not defined
    at <anonymous>:1:19
(anonymous) @ VM152:1

message = undefined;
const channelId = message.target.id.split('/')[0];

VM197:1 Uncaught TypeError: Cannot read properties of undefined (reading 'target')
    at <anonymous>:1:27
(anonymous) @ VM197:1

message = { target: 'hello' };
const channelId = message.target.id.split('/')[0];

VM278:1 Uncaught TypeError: Cannot read properties of undefined (reading 'split')
    at <anonymous>:1:37

[raucao]

There's nothing from Ember suppressing any exceptions afaik. It should just mean that the exception is being handled somewhere in the codebase. Probably a promise rejection handler or similar.

However, if a target cannot be expanded, that should throw an error before even trying to process a response, no? Although generally this shouldn't even happen when using the APIs correctly, since app developers shouldn't have to know about those Sockethub details IMO.

[silverbucket]

There's nothing from Ember suppressing any exceptions afaik. It should just mean that the exception is being handled somewhere in the codebase. Probably a promise rejection handler or similar.

Makes sense, will need to track down which promise swallowed it that should have a catch added.

However, if a target cannot be expanded, that should throw an error before even trying to process a response, no? Although generally this shouldn't even happen when using the APIs correctly, since app developers shouldn't have to know about those Sockethub details IMO.

Nothing failed to expand, like I said initially --

The target was not expanded because the completed join handler did not send back a copy of the same object that was sent.

... a completed job won't send back the exact same object that was sent, seems like just a waste, as the object exists on the senders end. If there is data sent back it's new data (e.g. a fetch RSS feed would have the callback contain the response, however a connect or join doesn't have a data response).

In the diff, you can see I swapped the creation of the activity object so we have the expanded object ready to be processed once the callback is received.

[raucao]

Nothing failed to expand, like I said initially --

The target was not expanded because the completed join handler did not send back a copy of the same object that was sent.

Either way, this API is breaking in a non-breaking PR, or has been broken recently without docs. It doesn't have to send back the object that was sent, but the callback function argument needs to be an object, as documented, and as it is in master (or as it was when Hyperchannel was last tested with Sockethub).

If that API changes, then Sockethub needs a new major version number and documentation about the breaking change.

Also, if there is an object being sent back sometimes, then I think it should not just assume that message in this particular case can only ever be an error message. That would not be a clean platform API design IMO, since you'd expect the callback arguments to be somewhat consistent between the various API methods. The way it used to be was that if the object contains an error property, then there was an error with that job, and the error message is contained within that property.

[silverbucket]

If that API changes, then Sockethub needs a new major version number and documentation about the breaking change.

We're still in alpha, no major version or breaking change documentation needed aside from what will already be during the final release.

It doesn't have to send back the object that was sent, but the callback function argument needs to be an object, as documented, and as it is in master (or as it was when Hyperchannel was last tested with Sockethub).

I realize this was how it was for earlier alphas, but does it need to be an object? Why? I'm not opposed to it, but it seemed entirely superfluous and more special code handling.

Also, if there is an object being sent back sometimes, then I think it should not just assume that message in this particular case can only ever be an error message.

Generally, yes, but in the case of a connect that would be the only case. The idea is that a connect doesnt have anything to say when it's successful, only if there's an error. But sure, the checks can be message && message.error to be safe.

The way it used to be was that if the object contains an error property, then there was an error with that job, and the error message is contained within that property.

That's still the way it is

[raucao]

We're still in alpha, no major version or breaking change documentation needed aside from what will already be during the final release.

The point is that if the doc isn't changed now, there's a good chance it's not being updated for the release. There is no reason not to update the docs when introducing breaking changes, since they need to be updated for the release anyway, and you don't want to miss any breaking changes in the release notes.

The idea is that a connect doesnt have anything to say when it's successful

I can imagine many things that you could send for successful connects, to inform clients about various statuses of the connected server for example. But either way, I think the API should be consistent between methods.

That said, we can also handle there not being a callback argument in the case of success, of course.

[silverbucket]

@raucao @galfert - I completely forgot to mention this, but Hyperchannel for some reason is connecting to Sockethub twice and I can't figure out how it's happening.

That seems to be why presence updates are not being handled (they are being sent from sockethub) and the rooms aren't going white once joined.

In other news, after more thought I decided to have the callback handler always return an AS of some sort, even on completion with no data requested (eg. join or connect) you receive the same object. I think this makes the most sense. I updated the Sockethub PR https://github.com/sockethub/sockethub/pull/779 and this PR to match that behavior, so you can update back branches before testing this one or if either of you want to look in to the double-connect issue.

[silverbucket]

Disregard what I said about multiple connections, that was actually no longer happening I was just in an old state when I was double-checking the behavior. Today it's not happening, rooms are being updated as expected once joined.

This PR is ready, however Travis builds are failing, some ember error I don't recognize. Can @raucao or @galfert take a look?

[galfert]

This PR is ready, however Travis builds are failing, some ember error I don't recognize. Can @raucao or @galfert take a look?

That looks like some async function calls that try to access functions/properties after the test suite has torn down the object already. Probably from some callback or event handler.

Might be from here: https://github.com/67P/hyperchannel/pull/290/files#diff-ad81c48aa3ddb00ba52d372bfda036bda4ff08a4b143569bce990dfc216a3042R15-R22 but I'm not sure.

[silverbucket]

@galfert @raucao I removed the listeners and the logger, not sure why that was causing a problem, but anyway tests pass now.

[silverbucket]

@raucao @galfert ping, I'd like to merge this and https://github.com/sockethub/sockethub/pull/779 soon if possible

[raucao]

@silverbucket Did you see my questions here? Maybe I missed something?

[silverbucket]

This PR is actually not necessary as we still return the object on callback after-all. It works fine against https://github.com/sockethub/sockethub/pull/779