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

Adds Backend subscriptions for WebSockets events, and applies them to Mobile #144

Open
wants to merge 31 commits into
base: dev
Choose a base branch
from

Conversation

lorenjohnson
Copy link
Member

@lorenjohnson lorenjohnson commented Jan 22, 2025

Features summary

  • Adds three subscriptions:
    • updates:  Streams new Messages, MessageThreads, and Notifications for the current user through a new Update grapqhl union type.
    • peopleTyping: Along with the paired peopleTyping mutation and usePeopleTyping hook which should be re-usable in Web, handles typing state. Is applied to either a messageThreadId, postId, or commentId.
    • comments: Streams new comments for a given postId or commentId (implicitly a parentCommentId). Currently applied only in PostDetails on Mobile for parent comments (no UI impact at the moment), perhaps has other possible near-term applications.
  • Uses HTTP/2 Server Side Events (SSE) vs WebSockets, which seems to be the preference of the community these days. SSE support on React Native is achieved through a poly (pony?) fill and seems to work, but is something I'm watching and giving this a bit more attention is part of follow-up.
  • There remains badge updating on Mobile which should be pretty simple :crossed_fingers::skin-tone-3:(https://a.slack-edge.com/production-standard-emoji-assets/14.0/apple-medium/[email protected]), and refinement to the underlying along the way as we polish things this month, but working well as is.

Focus recommendations for review

For the review there is a lot to pay attention to, but recommended focus is:

  • The subscription setup in the backend. Mostly api/graphql/index.js and api/graphql/makeSubscriptions.js.
  • The application of useSubscription in mobile in AuthRootNavigatorThreadusePeopleTyping, and PostDetails.
  • The current state of apps/mobile/urlq-shared/updates.js with regard to the Subscriptions.
  • The bridging between WebSocket isTyping and the peopleTyping subscription and mutation (small fix on Web for Messages isTyping)

Additional detail and follow-on development notes

Resolving Mobile UI badge updates and any fine-grained URQL caching updates are out-of-scope for this branch.

Messages conversion status:

  • commentAdded
  • messageAdded ("updates" subscription)
  • userTyping ("peopleTyping" mutation + subscription)
  • newThread ( ("updates" subscription)
  • newNotification ("updates" subscription)
  • newPost (or group subscription) -- will save for later consideration / iterations
  • Badge updating and possibly some minor URQL cache reconciliation remains for incoming updates, and will be left out of scope for this branch

Remaining dev todos:

  • Relay sockets event for peopleTyping from backend subscription so peopleTyping appears on Web
  • Remove unused subscriptions
  • Follow-up PR a bit later:
    • Look into a Graphql Yoga upgrade to get us on Single Connection vs Distinct Connect Server Side Events (SSE) for subscriptions. After initial research upgrade from v2.13 that we're on all the way to the current v5.x looks like it could go very quickly.
    • Test and refine reconnect/retry for SSE

@lorenjohnson lorenjohnson changed the title yoga+urql+rn subscriptions (wip) Adds backend subscriptions for WebSockets events, and applies them to Mobile Jan 24, 2025
@lorenjohnson lorenjohnson self-assigned this Jan 24, 2025
@lorenjohnson lorenjohnson changed the title Adds backend subscriptions for WebSockets events, and applies them to Mobile Adds Backend subscriptions for WebSockets events, and applies them to Mobile Jan 24, 2025
Copy link
Collaborator

@tibetsprague tibetsprague left a comment

Choose a reason for hiding this comment

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

cool!

@lorenjohnson
Copy link
Member Author

cool!

@tibetsprague aH yes, I kinda accidentally marked for review early. Will assign you and Tom again once it is ready ready sometime in the next day or so.

There is a bit more refinement coming with the final message implementations, but it's mostly here. It was pretty rough getting through the layers, but once things started working it became pretty satisfying. May still switch to WebSocket underneath instead of HTTP2/Events, not sure yet. It should mostly just be an adapter change if I do though. There are definitely some wins here, which like a lot of this stuff, we'll feel more later.

…ql return type to support Notification, MessageThread, and Message
… to userTyping WebSocket event. Fixes Messages isTyping in web. Clean-up of unused code and notes.
@lorenjohnson lorenjohnson marked this pull request as ready for review January 26, 2025 09:56
@@ -56,7 +56,7 @@ export default function TabsNavigator () {
<Tabs.Navigator {...navigatorProps}>
<Tabs.Screen name='Home Tab' component={HomeNavigator} />
<Tabs.Screen name='Search Tab' component={SearchNavigator} />
<Tabs.Screen name='Messages Tab' component={MessagesNavigator} />
<Tabs.Screen name='Messages Tab' component={MessagesNavigator} options={{ tabBarBadge: 999 }} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Whats this do?

Copy link
Member Author

Choose a reason for hiding this comment

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

A bookmark for badges to come. Quite unmissable in the UI


switch (update?.__typename) {
case 'Message': {
console.log('!!!! updates TODO: new Message. Increment Messages tab badge', result, args)
Copy link
Collaborator

Choose a reason for hiding this comment

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

??

Copy link
Member Author

Choose a reason for hiding this comment

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

As mentioned in the perhaps tl;dr description:), managing badges is out of scope in this PR and "coming soon". The logging, and the 999 badge on the Messages tab are meant as book marks.

Copy link
Collaborator

@thomasgwatson thomasgwatson left a comment

Choose a reason for hiding this comment

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

lgtm

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

Successfully merging this pull request may close these issues.

3 participants