Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type check conversations and events #87

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

demurgos
Copy link
Member

@demurgos demurgos commented Feb 8, 2018

This is a large PR introducing a change similar to the contact API: it moves the focus of the library on abstracting HTTP calls and guaranteeing the return values, and minimizing renaming or other normalizations.

The immediate benefit is that the skype events are now more rich: the API exposes the all the available data.
One of the downsides is that there is no more conversationId on message events: you have a conversationLink instead and have to retrieve the id manually. Next PRs should adds helpers to facilitate this.

@demurgos
Copy link
Member Author

demurgos commented Feb 13, 2018

@grigori-gru

As I see you're going to remove checking EventMessage.resourceType in the next version?
original comment

I'm responding here for context.

The change around the events is that the API would only emit two kinds of events: error and event:

    this.messagesPoller = new MessagesPoller(this.io, this.context);
    this.messagesPoller.on("error", (err: Error) => this.emit("error", err));

(source)

Currently, there are 4 events: error, event, Text and RichText.
The idea originally was to emit specific events so you could do api.on("Event/Call", ev => ...) to only handle calls for example. This never got fully implemented so you had to use the catch-all event event. The solution was to either add specific events on the API for each possible Skype event or just have the catch-all event (the current middle ground with a catch-all and 2 specific events is not optimal).
Updating all the api.on("Text", ev => ...) and api.on("RichText", ev => ...) by api.on("event", ev => ...) shouldn't be a big change: I believe you already had to use the catch-all event anyway.

The bigger change is around the event object. error events still pass an Error instance, but the event event now passes a SkypeEvent object instead of an EventMessage.
Or with code:

// Current version
api.on("event", (event: EventMessage) => {...});

// Version in this PR
api.on("event", (event: SkypeEvent) => {...});

This change means two things:

  • You get more events. The current version simply drops non-message events (mostly user presence). With this PR, every single event received from the network is emitted.
  • The events are closer to the "real" Skype API. This part is more related to the move toward a lower-level API (discussed in the v0.1.0 issue). The JSON messages received from the network are currently transformed to be easier to consume. The current version extracts identifiers from absolute URLs, parses some XML content but also ignores some fields. The version in this PR only checks for required fields, parses Date strings or enums, normalizes the case to camelCase. There are still some changes but this is a more direct representation of the Skype API.

Now, how do you deal with these new Skype events?
As you can see in the SkypeEvent definition, there are three main categories of events: EndpointPresenceEvent, UserPresenceEvent and NewMessageEvent.
You can differentiate them by looking at their resourceType property (which is an EventResourceType enum value).
UserPresenceEvent and EndpointPresenceEvent are related to the active sessions of your contacts. The first one represents online contacts regardless of their endpoint, the second on is about endpoints: a user can be online but connected with both his phone and computer at the same time for example (one user presence but two endpoints), these events are currently dropped. NewEventMessage is the closest to the current events, they hold a resource which are one of the possible MessageResource, which you can differentiate by looking at messageType (this corresponds to the current event.resource.type).

To clarify:

// Curent version
api.on("event", (event: EventMessage) => {
  // "ConversationUpdate" or "NewMessage"
  if (event.resourceType === "ConversationUpdate") {
    console.log("Received conversation update");
  } else { // "NewMessage"
    switch (event.resource.type) {
      case "RichText":
        console.log(event.resource.from); // A parsed user ID (MRI key)
        console.log(event.resource.content);
        console.log(event.resource.clientId); // Message ID assigned by the client
      default:
        // ...
    }
  }
});

// Version in this PR:
api.on("event", (event: SkypeEvent) => {
  switch (event.resourceType) {
    case EventResourceType.EndpointPresenceEvent:
      console.log("Connection to a new endpoint");
      break;
    case EventResourceType.UserPresenceEvent:
      console.log("New user connection");
      break;
    case EventResourceType.NewMessageEvent:
      switch (event.resource.messageType) {
        case "RichText":
          console.log(event.resource.from); // An absolute URL (a helper should be provided for this)
          console.log(event.resource.content);
          console.log(event.resource.clientMessageId); // Message ID assigned by the client (renamed)
        default:
          // ...
      }
      break;
  }
});

This PR needs at least the following changes:

  • enums should be usable as strings (easier usage in vanilla JS)
  • helpers should be provided to handle absolute URLs instead of ids
  • it should be tested with some real accounts to find missing events: as I write this, I notice that this PR may be missing the "ConversationUpdate" family of events.

@grigori-gru
Copy link
Contributor

@demurgos
Thanks for detailed explanation! I'd like to add some types to MessageResource. For example "ThreadActivity/AddMember" messagetype. How can I help you with it? Should I wait for closing this pullrequest or do smth else?

@demurgos
Copy link
Member Author

I think the easiest way to work on this would be to merge this first and then but I don't think I'll have time to finish it immediately. It will probably be done by the end of February.

Having most of the message types figured out would be great: it will probably be the main focus of the next version.

I started to collect some examples of JSON messages.
Until this PR is merged, could you post here some examples of events if you have them? (I usually swap some characters in identifiers and keys just in case).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants