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

Group message delivery status #372

Merged
merged 10 commits into from
Apr 24, 2024
Merged

Group message delivery status #372

merged 10 commits into from
Apr 24, 2024

Conversation

nplasterer
Copy link
Contributor

@nplasterer nplasterer commented Apr 19, 2024

Part of xmtp/libxmtp#516

  • Add enum field for delivery status that maps to ffi
  • Expose delivery status on messages
  • Expose delivery status filtering on find_messages
  • Expose message id on send

@nplasterer nplasterer self-assigned this Apr 19, 2024
@nplasterer nplasterer marked this pull request as ready for review April 23, 2024 05:57
@nplasterer nplasterer requested a review from a team as a code owner April 23, 2024 05:57
@@ -110,24 +111,20 @@ export class Group<
*/
async messages(
skipSync: boolean = false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Want to include skipSync in the options? You can do something like:

interface GroupMessagesOptions extends MessagesOptions {
skipSync?: boolean
}

And you can remove some duplicate code doing:

const {
skipSync = false
} = opts ?? {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd be curious @cameronvoell thoughts here. He added skipSync all over the code base so I kind of felt like it belonged outside of the messageOptions as it's more of a conscious action. But I'm totally fine with moving it into the options if we're cool with that.

Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency with other functions I think it's slightly better to keep it as a parameter the way it is.

Right now feels like the optional skipSync is the least bad way I've heard for having developers be generally aware that libxmtp group functions only return results from local data, and require a sync first if you want latest from network. Open to other suggestions for sure. But if we do keep it, I think we might as well have it passed as a parameter in a consistent way across functions.

Copy link
Contributor

@cameronvoell cameronvoell left a comment

Choose a reason for hiding this comment

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

looks good! left a comment with my suggestion for skipSync param

@nplasterer nplasterer merged commit 6551038 into beta Apr 24, 2024
5 of 6 checks passed
@nplasterer nplasterer deleted the np/delivery-status branch April 24, 2024 04:25
Copy link
Contributor

🎉 This PR is included in version 1.32.5-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link
Contributor

🎉 This PR is included in version 1.33.1-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link
Contributor

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants