Sigurd review comments

frontend/src/framework/Module.tsx

@@ -47,6 +47,7 @@ export class Module<StateType extends StateBaseType> {
     private _channelDefinitions: ModuleChannelDefinition[] | null;
     private _channelReceiverDefinitions: ModuleChannelReceiverDefinition[] | null;
 
+    // Create separate interface/type for ModuleOptions
     constructor({
         name,
         defaultTitle,

frontend/src/framework/ModuleContext.ts

@@ -85,6 +85,8 @@ export class ModuleContext<S extends StateBaseType> {
         });
     }
 
+    // Hmm... options object or parameters?
+    // In-place type or explicit type?
     usePublishChannelContents(options: {
         channelIdString: string;
         dependencies: any[];

frontend/src/framework/ModuleInstance.ts

@@ -38,6 +38,7 @@ export class ModuleInstance<StateType extends StateBaseType> {
     private _statusController: ModuleInstanceStatusControllerInternal;
     private _channelManager: ModuleChannelManager;
 
+    // Create separate interface/type for ModuleInstanceOptions?
     constructor({
         module,
         instanceNumber,

frontend/src/framework/internal/DataChannels/ModuleChannel.ts

@@ -3,18 +3,21 @@ import { KeyKind } from "@framework/DataChannelTypes";
 import { DataGenerator, ModuleChannelContent, ModuleChannelContentNotificationTopic } from "./ModuleChannelContent";
 import { ModuleChannelManager } from "./ModuleChannelManager";
 
+// rename to ChannelDefinition
 export interface ModuleChannelDefinition {
     readonly idString: string;
     readonly displayName: string;
     readonly kindOfKey: KeyKind;
 }
 
+//rename to ChannelNotificationTopic
 export enum ModuleChannelNotificationTopic {
     ContentsArrayChange = "contents-array-change",
     ContentsDataArraysChange = "contents-data-arrays-change",
     ChannelAboutToBeRemoved = "channel-about-to-be-removed",
 }
 
+// rename to Channel
 export class ModuleChannel {
     private _idString: string;
     private _displayName: string;
@@ -23,6 +26,7 @@ export class ModuleChannel {
     private _contents: ModuleChannelContent[] = [];
     private _subscribersMap: Map<ModuleChannelNotificationTopic, Set<() => void>> = new Map();
 
+    // utilize the ModuleChannelDefinition interface (+ manager)
     constructor({
         manager,
         idString,
@@ -66,6 +70,8 @@ export class ModuleChannel {
         this.notifySubscribers(ModuleChannelNotificationTopic.ContentsDataArraysChange);
     }
 
+    // Not a big fan of the parameter object here, use interface or type instead?
+    // use ModuleChannelContentDefinition interface + dataGenerator
     replaceContents(
         contentDefinitions: {
             idString: string;
@@ -118,6 +124,7 @@ export class ModuleChannel {
         }
     }
 
+    // rename to notifySubscribersOfChannelAboutToBeRemoved()
     beforeRemove(): void {
         this.notifySubscribers(ModuleChannelNotificationTopic.ChannelAboutToBeRemoved);
     }

frontend/src/framework/internal/DataChannels/ModuleChannelContent.ts

@@ -1,26 +1,35 @@
 import { DataElement, KeyType } from "@framework/DataChannelTypes";
 
+// rename to ChannelContentDefinition
 export interface ModuleChannelContentDefinition {
     idString: string;
     displayName: string;
 }
 
+// rename to ChannelContentNotificationTopic
 export enum ModuleChannelContentNotificationTopic {
     DataArrayChange = "data-array-change",
 }
 
+// rename to ChannelContentMetaData
 export interface ModuleChannelContentMetaData {
     ensembleIdentString: string;
     unit?: string;
     displayString?: string;
     preferredColor?: string;
 }
 
+// isn't the use of KeyType here a bit strange?
+// Should this type be renamed to DataGeneratorFunc?
+// Should we have a return type for this function?, meaning a defined type?
 export type DataGenerator = () => {
     data: DataElement<KeyType>[];
     metaData: ModuleChannelContentMetaData;
 };
 
+// rename to ChannelContent
+//
+// should this class be templated on KeyType
 export class ModuleChannelContent {
     private _idString: string;
     private _displayName: string;
@@ -29,6 +38,8 @@ export class ModuleChannelContent {
     private _cachedMetaData: ModuleChannelContentMetaData | null = null;
     private _subscribersMap: Map<ModuleChannelContentNotificationTopic, Set<() => void>> = new Map();
 
+    // Not a big fan of the parameter object here, few enough parameters to use more direct signature
+    // Use ModuleChannelContentDefinition interface instead + dataGenerator
     constructor({
         idString,
         displayName,
@@ -51,6 +62,7 @@ export class ModuleChannelContent {
         return this._displayName;
     }
 
+    // Shouldn't the type of dataGenerator be DataGenerator
     publish(
         dataGenerator: () => {
             data: DataElement<KeyType>[];

frontend/src/framework/internal/DataChannels/ModuleChannelManager.ts

@@ -8,6 +8,7 @@ export enum ModuleChannelManagerNotificationTopic {
     ReceiversChange = "receivers-change",
 }
 
+// rename to ChannelManager
 export class ModuleChannelManager {
     private readonly _moduleInstanceId: string;
     private _channels: ModuleChannel[] = [];
@@ -38,6 +39,7 @@ export class ModuleChannelManager {
         return this._moduleInstanceId;
     }
 
+    // utilize the ModuleChannelDefinition interface
     registerChannels(
         channelDefinitions: {
             readonly idString: string;
@@ -56,6 +58,7 @@ export class ModuleChannelManager {
         this.notifySubscribers(ModuleChannelManagerNotificationTopic.ChannelsChange);
     }
 
+    // Utilize the ModuleChannelReceiverDefinition interface
     registerReceivers(
         receiverDefinitions: {
             readonly idString: string;

frontend/src/framework/internal/DataChannels/ModuleChannelReceiver.ts

@@ -15,6 +15,7 @@ export enum ModuleChannelReceiverNotificationTopic {
     ChannelChange = "channel-change",
 }
 
+// rename to ChannelReceiver
 export class ModuleChannelReceiver {
     private readonly _manager: ModuleChannelManager;
     private readonly _idString: string;
@@ -26,6 +27,7 @@ export class ModuleChannelReceiver {
     private _subscribersMap: Map<ModuleChannelReceiverNotificationTopic, Set<() => void>> = new Map();
     private _subscribedToAllContents = false;
 
+    // utilize the ModuleChannelReceiverDefinition interface
     constructor({
         manager,
         idString,

frontend/src/framework/internal/DataChannels/hooks/useChannelReceiver.ts

@@ -14,11 +14,13 @@ export interface ChannelReceiverChannelContent<TKeyKinds extends KeyKind[]> {
     metaData: ModuleChannelContentMetaData;
 }
 
+// Have a look?
 export type ChannelReceiverReturnData<TKeyKinds extends KeyKind[]> = {
     idString: string;
     displayName: string;
     isPending: boolean;
     revisionNumber: number;
+    //activeChannel: ChannelWithSub | undefined; ???
 } & (
     | {
           channel: {
@@ -36,6 +38,8 @@ export type ChannelReceiverReturnData<TKeyKinds extends KeyKind[]> = {
       }
 );
 
+// Do we need an options object here? Only two parameters, could use direct signature
+// If we do, define a type/interface for it
 export function useChannelReceiver<TKeyKinds extends KeyKind[]>({
     receiver,
     expectedKindsOfKeys,