refactor(LSG): get rid of async in handlers; extract shared logic to base classes #1367
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release53 #1367 +/- ##
============================================
Coverage ? 56.71%
============================================
Files ? 397
Lines ? 72363
Branches ? 4389
============================================
Hits ? 41038
Misses ? 31219
Partials ? 106 ☔ View full report in Codecov by Sentry. |
ianshade
changed the title
nytamin
added
Contribution
| } | ||
| export type ObserverCallback<T, K extends keyof T> = (data: Pick<T, K> | undefined) => void | ||
|
|
||
| export type PickArr<T, K extends readonly (keyof T)[]> = Pick<T, K[number]> |
There was a problem hiding this comment.
Considering the very generic nature of this Type utility, should it perhaps live outside of this source file? Maybe in shared-lib, alongside our other type utilities?
| if (!col) throw new Error(`collection '${this._collectionName}' not found!`) | ||
| this._collectionData = col.find({ rundownId: this._curRundownId }) | ||
| await this.notify(this._collectionData) | ||
| protected changed(): void { |
There was a problem hiding this comment.
This member is protected, while a similar member in globalAdLibActionsHandler isn't. Would it make sense to standarize them?
| import { RundownId } from '@sofie-automation/corelib/dist/dataModel/Ids' | ||
|
|
||
| const PLAYLIST_KEYS = ['currentPartInfo', 'nextPartInfo'] as const | ||
| type Playlist = PickArr<DBRundownPlaylist, typeof PLAYLIST_KEYS> | ||
|
|
||
| export class GlobalAdLibActionsHandler |
There was a problem hiding this comment.
There seems to be a set of these classes that are effectively the same implementation, just with a bunch of specific Types for members they are handling (I'm seeing AdLibsHandler, AdLibActionsHandler, GlobalAdLibActionsHandler specifically). Do you think it would make sense to make them a single implementation? I don't see much difference betwen them TBH. Maybe they could all just extend a single base class?
| this.updateAndNotify() | ||
| } | ||
|
|
||
| protected updateAndNotify(): void { |
There was a problem hiding this comment.
Why is this member protected and not private?
| } | ||
| } | ||
|
|
||
| export abstract class PublicationCollection< |
There was a problem hiding this comment.
Do yo think it would make sense to put thse abstract classes to files of their own? I suppose they aren't really related to the WebSocketTopicBase as such?
| TPubSub extends CorelibPubSub | undefined, | ||
| TCollection extends keyof CorelibPubSubCollections | ||
| > { | ||
| export abstract class CollectionBase<T, TCollection extends keyof CorelibPubSubCollections> { |
There was a problem hiding this comment.
I realize this is a refactoring of the older class, but considering PublicationCollection has now been added to this file, do you think it would make sense to move it to it's own file, to avoid it growing?
| this._subscriptionPending = false | ||
| this.changed() | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| export interface Collection<T> { |
There was a problem hiding this comment.
I suppose this should go with CollectionBase to it's own file?
About the Contributor
This pull request is posted on behalf of TV 2 Norge
Type of Contribution
This is a:
Code improvement
Current Behavior
New Behavior
CollectionBaseclassPublicationCollectionclassinitmethods of those handlers and topics instead of all happening inLiveStatusServerupdatemethods are broken down into smaller ones, per each collection (tradeoff: a little extra boilerplate, but less indentation and better type safety)observerName)rundownId)Testing
Affected areas
This PR affects the Live Status Gateway
Time Frame
Not urgent, but it would be nice to have in release 52
Other Information
These changes were so far tested only on release50, with release51 LSGStatus