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

Promise support for MessageIdGenerator #11

Open
s-ueno opened this issue Jul 22, 2022 · 4 comments
Open

Promise support for MessageIdGenerator #11

s-ueno opened this issue Jul 22, 2022 · 4 comments
Assignees
Labels
enhancement New feature or request

Comments

@s-ueno
Copy link

s-ueno commented Jul 22, 2022

I am looking at BasicStorage.
We add and publish messages with addMessage and use MessageIdGenerator internally, but we sometimes ask the server to issue message IDs.

It would be helpful if it could be accepted asynchronously.

@supersnager supersnager self-assigned this Jul 26, 2022
@supersnager supersnager added the enhancement New feature or request label Jul 26, 2022
@supersnager
Copy link
Contributor

@s-ueno This is interesting feature.

Here are some thoughts. I'm curious what you think about it - how this relates to your case?

The feature implies that the addMessage also need to be asynchronous.
This is because we don't want to have a message without an id (which can often be used as a React key property in renders).
So the message should be added to the collection when the id arrives.
This approach generates another challenge - if we have more messages, the promise of the second message can be resolved before the first. So we need to enqueue messages to insert them to the array in proper order or add some counter.

This approach has two caveats: a user will not see the message until the promise is resolved and can first see the appearance of the second message and then the first message above it. This is bad UX.

The second approach is to add some temporary id, insert a message to the collection immediately and replace the id when a promise is resolved. In this case, the addMessage method can be synchronous.

In both approaches, we need to handle the case when the add message promise is rejected. The question is how?
In the first approach, the matter is simple - we get a promise from addMessage, so we can handle it by ourselves.
In the second, we probably need an additional callback parameter "onError" to the addMessageMethod.

Looking forward to reading your thoughts.

@s-ueno
Copy link
Author

s-ueno commented Jul 27, 2022

Hi, @supersnager
Thanks for your comment.

I have similar thoughts to the second approach.

The first approach, as you say, has a poor UX.
I am also concerned about implementation errors by providing asynchronous addMessage to the developer.

What is interesting is that instead of adding the ID in the addMessage, we delegate the issuing of the ID to the MessageIdGenerator.

This is because, apart from the process of adding a message on the UI, we delegate the issuance of the key associated with the message to the MessageIdGenerator.

However, since some developers may think that issuing IDs in addMessage is sufficient, another way to think of it is to issue events when some operation is performed on a message, such as addMessage or updateMessage.

export declare class EventHub {
  remove(callbacl)
  listen(e, callback)
}
export declare const Hub: EventHub ;

If these events can be subscribed to asynchronously, it would be possible to freely implement processes such as exchanging messages with the server via the event hub or intervening in the results with updateMessage.

Callbacks can be received by Promise.resolve to support both synchronous and asynchronous.

This would be in line with the concept of separating the ui from the backend.

PS.
I really like the concept of this library.

I like the fact that many services abstract the ui and just deliver data, whereas on the contrary, they abstract the data and logic and provide a typical UI in chat. I also particularly like the fact that it works fine with React 18.

@supersnager
Copy link
Contributor

@s-ueno Thanks for your comment, we are going in the right direction :)

If I understood you correctly, do you want to leave the message addition process as is and add an onAddMessage callback?
Then the MessageIdGenerator can generate a temporary id for the UI, and you can get the message id from the server in the callback and update the message?

something like this:

// Generate random ID for the UI
const groupIdGenerator = () => nanoid();
const messageIdGenerator = <T extends MessageContentType>(message: ChatMessage<T>) => nanoid();

const externalStorage = new BasicStorage({ groupIdGenerator, messageIdGenerator});

// Get the id from the server and update the message
externalStorage.addEventListener("onAddMessage", async (addedMessageWithGeneratedId:ChatMessage, conversationId: ConversationId) => {

  const messageIdFromServer = await fetch(...);
  addedMessageWithGeneratedId:ChatMessage.id = messageIdFromServer;
  externalStorage.updateMessage(addedMessageWithGeneratedId:ChatMessage);

});

<ChatProvider serviceFactory={serviceFactory} storage={externalStorage} config={{
              typingThrottleTime: 250,
              typingDebounceTime: 900,
              debounceTyping: true,
              autoDraft: AutoDraft.Save | AutoDraft.Restore
            }}>
                <App />
            </ChatProvider>

I have an additional question to understand your message flow.
How does it work? Do you have to get the id from a separate source?
Can't you get it from the response of your service's send method?

Additional thoughts on implementation:

  • updateMessage searches messages by id, so we can't update it if the id is changed (using a reference is not a good idea)
  • addMessage and onMessage must have an additional argument "supressCallback" because we probably don't want to retrieve the message id from the server for received messages (they already have the correct id)

@s-ueno
Copy link
Author

s-ueno commented Jul 29, 2022

Hi, @supersnager

I wanted to persist the ID of a sent message and share it with my members.

I want to be able to reference past messages with an immutable URL based on the message ID.

I was confused because sendMessage in @useChat is void and the job of issuing message IDs was outsourced to MessageIdGenerator.

<MessageInput
  onSend={(innerHtml, textContent, innerText, nodes) =>
    send(innerHtml, textContent, innerText, nodes)
  }  
const {
  sendMessage,
  .......
} = useChat();
const send = (
    innerHtml: any,
    textContent: any,
    innerText: any,
    nodes: NodeList
  ) => {
    if (activeConversation) {

      const message = new customChatMessage<MessageContentType.TextHtml>({
        id: "", // Id will be generated by storage generator, so here you can pass an empty string
        content: innerHtml,
        ..........
      });

      // todo sendMessage is void. I want the generated ID.
      sendMessage({
        message,
        conversationId: activeConversation.id,
        senderId: user.id,
      });

      // const generatedId = message.id;
      // postAsync(...)

However, as you say, MessageIdGenerator seems to be able to achieve this by returning the issued ID as is and making the send process asynchronous.

<MessageInput
  onSend={ async (innerHtml, textContent, innerText, nodes) =>
    await sendAsync(innerHtml, textContent, innerText, nodes)
  }  
  const {
    sendMessage,
    .......
  } = useChat();
const sendAsync = async (
    innerHtml: any,
    textContent: any,
    innerText: any,
    nodes: NodeList
  ) => {
    if (activeConversation) {

      const message = new customChatMessage<MessageContentType.TextHtml>({
        id: uuid(), // Set a unique ID here
        content: innerHtml,
        ..........
      });

      // MessageIdGenerator returns the ID as is.
      sendMessage({
        message,
        conversationId: activeConversation.id,
        senderId: user.id,
      });

      const generatedId = message.id;
      await postAsync(...)

Thank you very much for all your help.

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

No branches or pull requests

2 participants