-
Notifications
You must be signed in to change notification settings - Fork 0
Remove contentFrame from Client #29
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
base: main
Are you sure you want to change the base?
Conversation
| @@ -1 +1,2 @@ | |||
| type MessageId* = string | |||
| type Content* = seq[byte] | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The content may introduce broken changes, should we add version to facilitate the upgrades?
Also the app may choose different ways to encode the content, for example, with or without sds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The content may introduce broken changes, should we add version to facilitate the upgrades?
Ultimately Applications should be using an interop protocol such as ContentFrames, which handles versioning and type discrimination.
Also the app may choose different ways to encode the content, for example, with or without sds.
- Whether SDS is used is determined by the conversation type. Developers ought to not even know it exists.
- The proposed solution does not assume anything about the content. If developers want to wrap their content in another protocol, they are free to do so - It just has to be converted to
seq[bytes]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR relaxes the requirements and allows developers to choose any format.
If we think versioning and other features should be mandatory(which I could support) then we should close this PR and leave it as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think versioning is a MUST feature to deliver chat-sdk to 3rd party devs.
Without it, we need a lot of effort to communicate the broken changes, and could be hard to make any major release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whether SDS is used is determined by the conversation type. Developers ought to not even know it exists.
SDS brings some level of reliability, but also introduce burden like increased message size. There maybe other solutions that have different tradeoffs. I think it's better to leave space to devs to decide which reliability protocol to use in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I think we might be on different pages.
Content is the data supplied by the application to be transmitted by the chat protocol. Previously we required the content to be ConversationFrames. This PR allows applications to send seq[bytes]. This could be an encoded ContentFrame, binary data or whatever application encoded data the developer chooses.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think versioning is a MUST feature to deliver chat-sdk to 3rd party devs.
I completely agree. But "what" versioning are you referring to?
- Protocol versioning: Specific frame formats/processing.
- Content versioning: Application supplied data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think both the protocol versioning and app content versioning is required, but I'm not sure what's the best practice around such topics. Maybe another library like multiaddr, or documentation about such necessary parts. Anyway we can do this later.
Problem
Currently the
convo.sendMessageandonNewMessageassume content is formatted as a ContentFrame. As contentFrames are an independent/optional protocol the chat sdk should not require them. ref: #21Solution
This PR defines a type (
Content) asseq[byte]and updates all functions to use it. Clients now expectseq[bytes]as input messages.Note
sendMessageand callback. Handling it automatically within the api would improve the developer experience. This caused issues within the type system and will require more work. Pushing this step for now.