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

feat: add shouldPush to contentTypes messages #235

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions example/src/tests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ class NumberCodec implements JSContentCodec<NumberRef> {
fallback(content: NumberRef): string | undefined {
return 'a billion'
}

shouldPush(content: NumberRef): boolean {
return false
}
}

export type Test = {
Expand Down Expand Up @@ -808,6 +812,46 @@ test('register and use custom content types when preparing message', async () =>
return true
})

test('shouldPush for content types', async () => {
const bob = await Client.createRandom({
env: 'local',
codecs: [new NumberCodec()],
})
const alice = await Client.createRandom({
env: 'local',
codecs: [new NumberCodec()],
})

bob.register(new NumberCodec())
alice.register(new NumberCodec())

const bobConvo = await bob.conversations.newConversation(alice.address)
const aliceConvo = await alice.conversations.newConversation(bob.address)

await bobConvo.send('hi')

const messages = await aliceConvo.messages()

const message = messages[0]
assert(message.shouldPush === true, 'shouldPush should be true')

const prepped = await bobConvo.prepareMessage(
{ topNumber: { bottomNumber: 12 } },
{
contentType: ContentTypeNumber,
}
)

await bobConvo.sendPreparedMessage(prepped)

const messages2 = await aliceConvo.messages()
const message2 = messages2[0]

assert(message2.shouldPush === false, 'shouldPush should be false')

return true
})

test('calls preCreateIdentityCallback when supplied', async () => {
let isCallbackCalled = false
const preCreateIdentityCallback = () => {
Expand Down
1 change: 1 addition & 0 deletions ios/Wrappers/DecodedMessageWrapper.swift
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ struct DecodedMessageWrapper {
"senderAddress": model.senderAddress,
"sent": UInt64(model.sentAt.timeIntervalSince1970 * 1000),
"fallback": model.encodedContent.fallback,
"shouldPush": model.shouldPush
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kele-leanes I think here instead of calling model.shouldPush maybe you can copy how the test does it

		let message = try await MessageV2.encode(
			client: client,
			content: model.encodedContentt,
			topic: model.topic,
			keyMaterial: Data(model.keyMaterial),
			codec: model.encodedContent.contentType // MAP THIS TO A CODEC?
		)

then it would be message.shouldPush

Or something along these lines to get the push without having to make changes to the iOS sdk directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To do that I need to make MessageV2 and codecRegistry public and add keyMaterial to model which is not currently sent

Copy link
Contributor Author

@kele-leanes kele-leanes Feb 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and making this method async/await would add more complexity where this method is called

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no issue with MessageV2 and codecRegistry being public. @nakajima do you have any thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

besides that, since the codec registry in RN lives on the JS side, the MessageV2.encode method can not access the codecs from RN.
e.g: If I register a custom codec, which returns true from the shouldPush method, the native side can’t get this flag.
Is the reason why I suggest sending the value encoded as in this PR or sending it as a part of a SendOption object

Copy link
Contributor Author

@kele-leanes kele-leanes Feb 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the behavior in iOS, shouldPush is already established and calculated based on the CodecTypes existing on the iOS side. However, in React Native (RN), CodecTypes are generated and stored on the JavaScript side. Consequently, the determination of this flag should occur on the JS side and then communicated to iOS. I've devised two solutions:

  1. Include the shouldPush value as part of the EncodedContent using the proto, similar to how we currently send the fallback. This value can then be decoded and set when a MessageV2 is created. (Link to PR)

  2. Send the shouldPush value as a plain Boolean, as proposed in this PR, and include it as an optional part of the SendOption.

The latter approach deviates from the convention of maintaining methods as similar as possible across platforms.

Copy link
Contributor

@richardhuaaa richardhuaaa Feb 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha! Thanks for explaining.

So I don't think we should do (1) - it muddies the protocol, and the protos are shared across all of the platforms as well as sent over the network.

in React Native (RN), CodecTypes are generated and stored on the JavaScript side

More of a long-term question/not blocking this PR, but why is this the case? Is there a reason we can't push that down to the native platform?

The latter approach deviates from the convention of maintaining methods as similar as possible across platforms.

So the issue is that we are deriving shouldPush from the non-encoded content, however the SDK is structured so that only the encoded content is sent down today, which means iOS is unable to derive shouldPush? If so, then does that mean RN Android has the same issue and would also benefit from this?

It seems like we have no choice with the current design but to do the sendOption thing even if it's a little ugly. Another question - given the contentType is in the SendOptions already, would it be feasible to have iOS construct the codec based on the content type, and then determine the shouldPush value from that? That way we are not giving app developers a way to set shouldPush directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More of a long-term question/not blocking this PR, but why is this the case? Is there a reason we can't push that down to the native platform?

I'm not fully aware of the impact of this change, but the structure of a contentType has methods that you can not pass down to the native platform. This would solve our issues

Would it be feasible to have iOS construct the codec based on the content type, and then determine the shouldPush value from that?

We are not able to construct the codec since the contentType resides on the JS side. Specifically the custom ones. When the platform (in this case iOS) is trying to get the codec from a contentTypeId it searches throughout the generics codecs which are on the native side.

These changes must be added to the Android as well

Copy link
Contributor

@richardhuaaa richardhuaaa Feb 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. In that case it seems that adding shouldPush to the SendOptions is the only way for now. Perhaps we can name it something that makes it clear that it is internal-use only, like __shouldPush or something.

Long-term, there will probably be more cases like this where the RN SDK wants some functionality from the iOS SDK that we don't actually want to expose publicly. I wonder if it makes sense to define an iOS type/interface that is internal-use only, that we don't document or provide any guarantees about publicly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved in ccceabe

]
}

Expand Down
14 changes: 10 additions & 4 deletions ios/XMTPModule.swift
Original file line number Diff line number Diff line change
Expand Up @@ -226,14 +226,16 @@ public class XMTPModule: Module {
).toJson()
}

AsyncFunction("sendEncodedContent") { (clientAddress: String, topic: String, encodedContentData: [UInt8]) -> String in
AsyncFunction("sendEncodedContent") { (clientAddress: String, topic: String, encodedContentData: [UInt8], shouldPush: Bool) -> String in
guard let conversation = try await findConversation(clientAddress: clientAddress, topic: topic) else {
throw Error.conversationNotFound("no conversation found for \(topic)")
}

let encodedContent = try EncodedContent(serializedData: Data(encodedContentData))

let options = SendOptions(contentType: encodedContent.type, shouldPush: shouldPush)

return try await conversation.send(encodedContent: encodedContent)
return try await conversation.send(encodedContent: encodedContent, options: options)
}

AsyncFunction("listConversations") { (clientAddress: String) -> [String] in
Expand Down Expand Up @@ -390,15 +392,19 @@ public class XMTPModule: Module {
AsyncFunction("prepareEncodedMessage") { (
clientAddress: String,
conversationTopic: String,
encodedContentData: [UInt8]
encodedContentData: [UInt8],
shouldPush: Bool
) -> String in
guard let conversation = try await findConversation(clientAddress: clientAddress, topic: conversationTopic) else {
throw Error.conversationNotFound("no conversation found for \(conversationTopic)")
}
let encodedContent = try EncodedContent(serializedData: Data(encodedContentData))

let contentType = encodedContent.type

let prepared = try await conversation.prepareMessage(
encodedContent: encodedContent
encodedContent: encodedContent,
options: SendOptions(contentType: contentType, shouldPush: shouldPush)
)
let preparedAtMillis = prepared.envelopes[0].timestampNs / 1_000_000
let preparedData = try prepared.serializedData()
Expand Down
8 changes: 6 additions & 2 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -235,12 +235,14 @@ export async function sendWithContentType<T>(
} else {
const encodedContent = codec.encode(content)
encodedContent.fallback = codec.fallback(content)
const shouldPush = codec.shouldPush(content)
const encodedContentData = EncodedContent.encode(encodedContent).finish()

return await XMTPModule.sendEncodedContent(
clientAddress,
conversationTopic,
Array.from(encodedContentData)
Array.from(encodedContentData),
shouldPush
)
}
}
Expand Down Expand Up @@ -287,11 +289,13 @@ export async function prepareMessageWithContentType<T>(
}
const encodedContent = codec.encode(content)
encodedContent.fallback = codec.fallback(content)
const shouldPush = codec.shouldPush(content)
const encodedContentData = EncodedContent.encode(encodedContent).finish()
const preparedJson = await XMTPModule.prepareEncodedMessage(
clientAddress,
conversationTopic,
Array.from(encodedContentData)
Array.from(encodedContentData),
shouldPush
)
return JSON.parse(preparedJson)
}
Expand Down
2 changes: 2 additions & 0 deletions src/lib/ContentCodec.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { content } from '@xmtp/proto'

Check warning on line 1 in src/lib/ContentCodec.ts

View workflow job for this annotation

GitHub Actions / lint

There should be at least one empty line between import groups
import { ReadReceiptCodec } from './NativeCodecs/ReadReceiptCodec'

Check warning on line 2 in src/lib/ContentCodec.ts

View workflow job for this annotation

GitHub Actions / lint

'ReadReceiptCodec' is defined but never used
import { TextCodec } from './NativeCodecs/TextCodec'

Check warning on line 3 in src/lib/ContentCodec.ts

View workflow job for this annotation

GitHub Actions / lint

'TextCodec' is defined but never used
import { ReplyCodec } from './NativeCodecs/ReplyCodec'

Check warning on line 4 in src/lib/ContentCodec.ts

View workflow job for this annotation

GitHub Actions / lint

`./NativeCodecs/ReplyCodec` import should occur before import of `./NativeCodecs/TextCodec`

Check warning on line 4 in src/lib/ContentCodec.ts

View workflow job for this annotation

GitHub Actions / lint

'ReplyCodec' is defined but never used

export type EncodedContent = content.EncodedContent
export type ContentTypeId = content.ContentTypeId
Expand Down Expand Up @@ -101,6 +101,7 @@
encode(content: T): EncodedContent
decode(encodedContent: EncodedContent): T
fallback(content: T): string | undefined
shouldPush(content: T): boolean
}

export interface NativeContentCodec<T> {
Expand All @@ -109,6 +110,7 @@
encode(content: T): NativeMessageContent
decode(nativeContent: NativeMessageContent): T
fallback(content: T): string | undefined
shouldPush(content: T): boolean
}

export type ContentCodec<T> = JSContentCodec<T> | NativeContentCodec<T>
12 changes: 9 additions & 3 deletions src/lib/DecodedMessage.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import { Client } from './Client'
import {
ContentCodec,

Check warning on line 3 in src/lib/DecodedMessage.ts

View workflow job for this annotation

GitHub Actions / lint

'ContentCodec' is defined but never used
JSContentCodec,
NativeContentCodec,
NativeMessageContent,
} from './ContentCodec'
import { ReplyCodec } from './NativeCodecs/ReplyCodec'

Check warning on line 8 in src/lib/DecodedMessage.ts

View workflow job for this annotation

GitHub Actions / lint

'ReplyCodec' is defined but never used
import { TextCodec } from './NativeCodecs/TextCodec'

Check warning on line 9 in src/lib/DecodedMessage.ts

View workflow job for this annotation

GitHub Actions / lint

There should be at least one empty line between import groups

Check warning on line 9 in src/lib/DecodedMessage.ts

View workflow job for this annotation

GitHub Actions / lint

'TextCodec' is defined but never used
import { Buffer } from 'buffer'

export class DecodedMessage<ContentTypes = any> {
Expand All @@ -18,6 +18,7 @@
sent: number // timestamp in milliseconds
nativeContent: NativeMessageContent
fallback: string | undefined
shouldPush: boolean

static from<ContentTypes>(
json: string,
Expand All @@ -32,7 +33,8 @@
decoded.senderAddress,
decoded.sent,
decoded.content,
decoded.fallback
decoded.fallback,
decoded.shouldPush
)
}

Expand All @@ -45,6 +47,7 @@
sent: number // timestamp in milliseconds
content: any
fallback: string | undefined
shouldPush: boolean
},
client: Client<ContentTypes>
): DecodedMessage<ContentTypes> {
Expand All @@ -56,7 +59,8 @@
object.senderAddress,
object.sent,
object.content,
object.fallback
object.fallback,
object.shouldPush
)
}

Expand All @@ -68,7 +72,8 @@
senderAddress: string,
sent: number,
content: any,
fallback: string | undefined
fallback: string | undefined,
shouldPush: boolean
) {
this.client = client
this.id = id
Expand All @@ -78,6 +83,7 @@
this.sent = sent
this.nativeContent = content
this.fallback = fallback
this.shouldPush = shouldPush
}

content(): ContentTypes {
Expand Down
11 changes: 11 additions & 0 deletions src/lib/NativeCodecs/ReactionCodec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,4 +35,15 @@ export class ReactionCodec implements NativeContentCodec<ReactionContent> {
return undefined
}
}

shouldPush(content: ReactionContent): boolean {
switch (content.action) {
case 'added':
return true
case 'removed':
return false
default:
return false
}
}
}
4 changes: 4 additions & 0 deletions src/lib/NativeCodecs/ReadReceiptCodec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,8 @@ export class ReadReceiptCodec
fallback(content: object): string | undefined {
return undefined
}

shouldPush(content: object): boolean {
return false
}
}
4 changes: 4 additions & 0 deletions src/lib/NativeCodecs/RemoteAttachmentCodec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,8 @@ export class RemoteAttachmentCodec
fallback(content: RemoteAttachmentContent): string | undefined {
return `Can’t display "${content.filename}". This app doesn’t support attachments.`
}

shouldPush(content: RemoteAttachmentContent): boolean {
return true
}
}
4 changes: 4 additions & 0 deletions src/lib/NativeCodecs/ReplyCodec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,8 @@ export class ReplyCodec implements NativeContentCodec<ReplyContent> {
}
return 'Replied to an earlier message'
}

shouldPush(content: ReplyContent): boolean {
return true
}
}
4 changes: 4 additions & 0 deletions src/lib/NativeCodecs/StaticAttachmentCodec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,8 @@ export class StaticAttachmentCodec
fallback(content: StaticAttachmentContent): string | undefined {
return `Can’t display "${content.filename}". This app doesn’t support attachments.`
}

shouldPush(content: StaticAttachmentContent): boolean {
return true
}
}
4 changes: 4 additions & 0 deletions src/lib/NativeCodecs/TextCodec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,8 @@ export class TextCodec implements NativeContentCodec<string> {
fallback(content: string): string | undefined {
return content
}

shouldPush(content: string): boolean {
return true
}
}
Loading