-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Conversation
Dug into this for a few hours now, still blocked. It turns out while the new Turns out I reached out in the Slack channel and hope to get a response and release as soon as SF wakes up. |
how problematic do you think it would be to fork and change the dependency for now? |
Thanks for digging into this man, hopefully we can get resolution soon (just a heads up tomorrow is a US holiday so those folks might be offline, not sure :/) |
PR to update was merged! apollographql/apollo-server#407 Just waiting for npm publish now 👌 |
|
Fixed that, fixed some more bugs, upgraded some more things and now message subscriptions are working again 🎉 Onwards to the actual task of this PR: notifications subscriptions |
W00t you can now listen to new notifications! 🎉 This turned out to be a bit harder than originally anticipated because to listen to notifications of a user we need to, of course, know who the user is—the issue is that when connecting over websockets (of course, again) none of the HTTP express middlewares run so we have no clue if a user is authenticated or not. Essentially, I had to do all of that manually to then pass the current user (or not) into the GraphQL context but it's done now so you can listen to new notifications for yourself! (see commit cb9e608 for that part) Now going to do some cleanup and then the frontend side of things 🔥 |
@mxstbr - just pushed the initial setup for usersNotifications join table; I am going to walk my pup and head to Bryn's. After that I'll handle the following logic (unless you were planning to?):
I'm not really sure how to do an initial population of this table through the generators / it'll be a bit of work :) |
Here's my proposed changes to notifications for right now that will minimally impact our time to launch (~1 day?) but will dramatically reduce our debt in the future. I propose we think of all notifications as the following story: (An) Actor(s) triggers an Event which impacts an Entity which notifies Recipients. Examples:
We store this notification-level data on the notifications table. This will look like this: {
id: id
actors: [userId]
event: eventType_eventValue (e.g. thread_created, channel_made_private)
entity {
type: enum string
id: id
}
attachments: [ Attachments ]
} Attachments can be anything, which will allow us a ton of flexibility to put any arbitrary data structure in a notification that we want. E.g. imagine you get a notification that a community owner just created a new channel; what if we could have an attachment like: {
type: channelCreated
data: channelId
} and then on the client side we can iterate through the attachments to render a button in the notification to join that channel! In addition, we create a join table usersNotifications which stores metadata related to a specific user's relationship with a notification. It will look like this: {
id: id
userId: id
notificationId: id
isSeen: boolean
isRead: boolean
} What this looks like in practiceBryn creates a new thread in a channel where I am a member.
So the resulting objects in the database are: Notification: {
id: '1234567890'
actors: [ bryns-unique-userid ]
event: 'thread_created'
entity {
type: 'channel'
id: channel-id-for-new-thread
}
attachments [
{
type: 'thread'
data {
title
excerpt
}
}
]
} UsersNotifications (generated for all eligible recipients): {
id: uuid
userId: some-recipient-uid
notificationId: '1234567890'
isSeen: false
isRead: false
} Backend PipelineAn event occurs on the clientside - let's keep the same example of a thread being created. That thread gets stored in the db which will trigger the following chain of events:
{
type: 'thread_created'
entity {
type: 'channel'
id: channel-id-for-new-thread
}
}
GraphQLThe nice thing about this proposed setup is that we have much more powerful filtering options. We can get events by entity (e.g. show me all notifications for the channels I own). We can also do cool things like: get me all notifications for new threads that I saw but didn't click on (e.g. filtering by seen or unseen, as well as by notification type). Notification bundlingWe can re-use a lot of the same bundling logic we already have - basically loop through a user's notifications, group them by the entity type + id, and merge the actors from those two objects. Based on the merges that happen in here, we can modify any strings needed (e.g. multi-actor: Brian and Max posted new threads in Channel X; single-actor: Brian posted a new thread in Channel X, Thread Title) AttachmentsAttachments could be anything in the future, as noted above. In the short term we'll probably have attachments for:
In the medium term we could have attachments for:
In the long term if we have polls, or events, or video streaming, or AMAs, or anything like that, we could handle those here. Some examples: Thread preview {
attachmentType: 'thread'
data: {
title: Title
excerpt: Excerpt
}
} Media message {
attachmentType: 'media'
data: {
url: fileUrl.png
}
} Pending User in Private Channel You Own {
attachmentType: 'pendingChannelUser'
data: {
userId: id
channelId: id
}
} Of course with gql we can resolve a lot of ids and return full channel/user objects :) Proposed notifications for v2 launch
Proposed fast follow notifications
|
I think we have a few paths to shipping:
I personally am a fan of 1A. V1 of Spectrum already has no notifications, so shipping v2 without notifications is still a huge win anyways. I think it will take 1-2 extra days of work to rework our current notifications logic to be a bit more general purpose based on the proposal above, which we could ship shortly after launching v2. If we go with option 2, I still think it'll take several more hours of work to build + test, whether that's on max or I we can decide tonight. Let's discuss when you're awake @mxstbr and @uberbryn |
We agreed to go with 1A above and make this happen first thing after launch. I've disabled notifications client-side and server-side temporarily. Merging this so it doesn't go out of date. |
Haven't managed the upgrade to the new (and unreleased)
subscription-transport-ws
yet, ran into some weird bugs I couldn't resolve. (probably just me being tired)I've reset that and will take another stab at it fresh and clean first thing tomorrow to get that finished so I can look at DM notifications.