Emit events when setting unread notifications #2748
Conversation
| @@ -127,6 +128,7 @@ export enum RoomEvent { | |||
| OldStateUpdated = "Room.OldStateUpdated", | |||
| CurrentStateUpdated = "Room.CurrentStateUpdated", | |||
| HistoryImportedWithinTimeline = "Room.historyImportedWithinTimeline", | |||
| UnreadNotifications = "Room.UnreadNotifications", | |||
There was a problem hiding this comment.
it bothers me that we mix Subject.lowerCamelCase and Subject.UpperCamelCase, ftr. Personally I prefer lowerCamelCase, but that seems to be outvoted here.
| @@ -1193,8 +1206,17 @@ export class Room extends ReadReceipt<EmittedEvents, RoomEventHandlerMap> { | |||
| * @returns The notification count, or undefined if there is no count | |||
| * for this type. | |||
| */ | |||
| public getThreadUnreadNotificationCount(threadId: string, type = NotificationCountType.Total): number | undefined { | |||
| return this.threadNotifications.get(threadId)?.[type]; | |||
| public getThreadUnreadNotificationCount(threadId: string, type = NotificationCountType.Total): number { | |||
There was a problem hiding this comment.
this looks like a breaking change - the function is not flagged as experimental, it's public, and it has a behavioural change.
There was a problem hiding this comment.
It should have been marked as experimental. And this is only behind a labs flag.
I believe this should not be marked as a breaking change.
There was a problem hiding this comment.
The js-sdk doesn't have a concept of labs flags though, and we can't guarantee that people aren't using it. There's other breaking changes in the SDK already landed anyways, so I think we can just apply the label here and move on.
Co-authored-by: Travis Ralston <travisr@matrix.org>
|
@turt2live the Typescript Strict Error CI has gone a bit coocoo. Besides that I believe I've adressed all your comments |
|
@turt2live again, this change is NOT a breaking change. This function is behind a labs flag, and on top of that it has never made it to a release as it was introduced 7 days ago. I will be removing the label |
Paves the way for element-hq/element-web#23192
Checklist
This change is marked as an internal change (Task), so will not be included in the changelog.