Potential problem in api.ts

[kfatehi]

In the following snippet you can see that it is assumed that ev.resource.from.username is defined, but then, the existence of ev.resource is checked. So if ev.resource is definitely there, why check for it? Should the conditional be changed, then, to not assume ev.resource.from.username is a true object path in every event?

For context, I am looking at this because I'm actually interested in self-sent messages. I'm puzzled as to why there is a comment here saying there would be an infinite-loop. I'm using the generic event event in order to pick up self-sent messages now as a result, although I'd have preferred the Text/RichText ones.

https://github.com/ocilo/skype-http/blob/08666d19e5fcf47702da3ccdba493619d3c09722/src/lib/api.ts#L93-L101

    // Prevent infinite-loop (echo itself)
    if (ev.resource.from.username === this.context.username) {
      return;
    }

    if (ev && ev.resource && ev.resource.type === "Text") {
      this.emit("Text", ev.resource);
    } else if (ev && ev.resource && ev.resource.type === "RichText") {
      this.emit("RichText", ev.resource);

[mitchcapper]

@kfatehi good catch. That should be fixed two things: ) The ev check to make sure it exists should be before the infinite loop code (if that code is to stay) ) The code itself basically prevents the Text/RichText events from being emitted if the user sent the message. The standard bot demo replies to all Text events with a reply. This would cause an issue where the bot would try to reply to itself. One could argue implementers should just check to make sure the events are not self events before replying. We could than remove the infinite loop check completely. A better option may be to have a "self" listen option to the options. If it is true events for our own events are generated, if not they are not.

@demurgos do you have an opinion?

@kfatehi for a temporary (or forever) work around instead of listening to the Text event listen to the generic "event" event. It will always emit (even for self events), I use this to grab my own messages as well.

[kfatehi]

• The facebook chat api package IIRC uses the "self listen" option (api.setOptions({selfListen: true}))
• The signal chrome app which I ported to node.js actually emits a "sent" event, which I found really convenient.

Otherwise I think it's completely logical to have the app developer filter out self-sent messages like you've suggested.

That code should be taken out IMO considering it only makes sense for the contrived example of an echo bot -- it should have not moved into the library itself IMHO.

And indeed I am using event now as such, kind of modelling the way Signal does it. It's working nicely for me.

      api.on("event", (ev) => {
        //console.log(JSON.stringify(ev, null, 2));

        if (ev && ev.resource && ev.resource.type === "Text" || ev.resource.type === "RichText") {
          if (ev.resource.from.username === api.context.username) {
            // the lib currently hides this kind from us. but i want it.

            this.emit('sent', ev);
          } else {
            this.emit('message', ev);
          }

        }
      });

[demurgos]

You are right, this is a problem. The fact that the server acknowledges the reception of the message is an important event. Currently, this code does wrong assumptions as you already noted (and isn't even used by the echo bot).

I like the idea of the "sent" event but this would be a custom event. I am not sure how to handle it exactly: should there be a single "sent" event for all the kinds of messages or not ("Sent/Text", "Sent/RichText", etc.). The other solution is to do something like the Facebook chat API and pass the events as-is with an option to eventually prevent self messages. I think that having a "selfListen" option that defaults to true would produce the most predictable behavior. If one of you wants to do a PR, I'd accept it.

[mitchcapper]

So right now we do no filtering on event only on message. Do we want to filter events as well? I agree the FB style would be fairly straightforward.

[demurgos]

The issue with the redundant checks was solved. The main issue still there though, I may take a look at it during the next days.