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

Provide more intuitive APIs #28

Closed
akiroz opened this issue Feb 9, 2017 · 14 comments
Closed

Provide more intuitive APIs #28

akiroz opened this issue Feb 9, 2017 · 14 comments

Comments

@akiroz
Copy link
Contributor

akiroz commented Feb 9, 2017

I think much more of the client logic can be encapsulated into the SDK.... It's currently extremely hard to use.
IMO, the SDK's API should be based on ease of use and not ease of implementation..... :(

Things like "list of conversations that belongs to the current user" and "current conversation" should be part of the SDK, it's unlikely that a chat app will not have a list of chats or more than 1 current chat.

Some APIs are quite chaotic like: userConversation.$transient.conversation.$transient.last_message.body it almost seem like a hack / workaround.

@chpapa
Copy link

chpapa commented Feb 9, 2017

@akiroz Could you help list out the hack you run into, and also features that you wish the SDK have? so we can have a concrete discussion on shall we / how to include each features.

@chpapa
Copy link

chpapa commented Feb 9, 2017

I agree this "userConversation.$transient.conversation.$transient.last_message.body" looks crazy....

@rickmak thought?

@akiroz
Copy link
Contributor Author

akiroz commented Feb 9, 2017

The main feature is the typing indicator, current API allows me to send "start" and "stop" typing events but to know that I need to put the stream of input-field change events through a debounce loop.

Others are mostly the amount of "glue code" I need to write to get commonly used data.

It would be nice if the SDK handled the app's state to some degree since most chat apps will have a view of the current conversation, it could probably have something like activateConversation() which accepts a map of:

  • conversationID: the conversation to set as the current one the user is interacting with.
  • onMessages: a callback function that provides a list of messages that should be rendered on screen, the SDK should handle loading the initial messages, pubsub events (with duplication) and loading older messages automatically with user config-able behavior.
  • onTypeingEvent: a callback function that's called when some user typing event occurs (e.g. start/stop typing)

The developer should be able to access the current conversation with skygearChat.currentConversation much like skygear.currentUser with APIs like activateConversation(...) and deactivateConversation() acting like login() and logout()

@chpapa

@akiroz
Copy link
Contributor Author

akiroz commented Feb 9, 2017

Since the point of a chat SDK is to help you write a chat app, it should handle the most difficult parts of the job (i.e. async events handling, data synchronization and user behaviour detection in the case of typing)

@chpapa
Copy link

chpapa commented Feb 9, 2017

Let me try to summarize the exact suggestions, please try to add to the list @akiroz

  1. Utility functions to fire typing indicator event for textbox
  2. Message history management
  3. See if anyway we can make something like this looks better / easier to access: userConversation.$transient.conversation.$transient.last_message.body

For having conversation as a state, I think we might need to think about it carefully, it might not be very suitable for many cases, maybe some sort of utility functions / class on top of the skygear chat SDK?

@akiroz
Copy link
Contributor Author

akiroz commented Feb 9, 2017

Number 2 should be async message events handling. Currently I need to fetch the initial list of messages then subscribe to a channel for new messages and when new messages come in I need to put them in the correct order based on sent time and duplicate messages.

I think we can provide state management while still keeping the low-level access methods we currently have, there's no conflict. Most applications will probably have to manage the list of messages... IMO

@akiroz
Copy link
Contributor Author

akiroz commented Feb 9, 2017

@chpapa

We could probably have something like:

var messages = new ManagedMessageList({
  initialFetch: 50,
  subscribeNewMessages: true,
  syncOnReconnect: true,
});

messages => []Message

messages.on('update', function() {});

messages.fetchOlder(20)

@akiroz
Copy link
Contributor Author

akiroz commented Feb 10, 2017

Proposal for a typing indicator:

import skygearChat from 'skygear-chat';

// http://stackoverflow.com/questions/36871299/how-to-extend-function-with-es6-classes
class ExtensibleFunction extends Function {
  constructor(f) {
    return Object.setPrototypeOf(f, new.target.prototype);
  }
}

// TypingIndicator.js
export default class extends ExtensibleFunction {
  constructor(conversation, debounceTime = 3000) {
    super(() => {
      if(this.debounceTimer) {
        this.startTyping();
      } else {
        this.resetTimer();
      }   
    }); 
    this.conversation = conversation;
    this.debounceTime = debounceTime;
    this.debounceTimer = null;
  }
  startTyping() {
    this.debounceTimer = setTimeout(
      this.stopTyping,
      this.debounceTime
    );  
    skygearChat.sendTypingIndicator(
      this.conversation, 'begin'
    );  
  }
  resetTimer() {
    clearTimeout(this.debounceTimer);
    this.debounceTimer = setTimeout(
      this.stopTyping,
      this.debounceTime
    );  
  }
  stopTyping() {
    this.debounceTimer = null;
    skygearChat.sendTypingIndicator(
      this.conversation, 'finished'
    );  
  }
}

Usage:

import TypingIndicator from 'TypingIndicator.js';
const typing = new TypingIndicator(currentConversation);
<input type=text onChange=typing />

@chpapa
Copy link

chpapa commented Feb 10, 2017

The typing indicator have been proposed at the design phase of the chat SDK as a utility function but for some reason it didn't make it to the final release, any reason or feedback? @rickmak I have created #30

@chpapa
Copy link

chpapa commented Feb 10, 2017

Actually, I still think the ManagedMessageList you proposed above @akiroz would be something we expect it to handle offline sync (message history management), maybe I was 先入為主 as it was kind of discussed at the beginning but left out in the working scope.

Anyway I have created #31 for that. And I put it into two features.

Assume if there are no more idea I'm closing this issue, but please help open new one (or reopen this one for discussion purpose) if you have any other thoughts on the problem you had! @akiroz

@rickmak
Copy link
Member

rickmak commented Feb 10, 2017

I don't get why you needed to use userConversation.$transient.conversation.$transient.last_message.body

I agree the current is more a wrapper around feature. And as per my understanding, we will give the freedom for developer on how to implement the chat by providing low level feature. And the demo is the showcase of our suggested way doing common usecase. And I would like add utility with demo using them. So I think @akiroz work on react-chat-demo is good way to collect the need of utility function. We can review the code in action and discuss which part of code can be include in core as utility. Or just left it in demo as suggestion.

@chpapa
Copy link

chpapa commented Feb 10, 2017

@rickmak may we have your input on...

  1. What's a better alternative to userConversation.$transient.conversation.$transient.last_message.body? Or what problem u see in it?

  2. I agree looking at code in action is good, maybe when you (or someone else?) review the code from @akiroz on demo, can also suggest other patterns you saw?

Regarding what to put as utility function in SDK, what to put in demo, is a trade-off -- avoid overloading developer's mind when they look at the API doc / guide with hundreds of apis, while make it easy to discover something...

@chpapa chpapa reopened this Feb 10, 2017
@akiroz
Copy link
Contributor Author

akiroz commented Feb 10, 2017

I have a list of chats in the side, internally I keep an array of UserConversations.
Each item on the list needs to show the last message.

@chpapa
Copy link

chpapa commented Feb 20, 2017

Seems we have concluded the suggestion and created actionable items #31 and #30 ; Let's close this issue now.

@chpapa chpapa closed this as completed Feb 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants