-
Notifications
You must be signed in to change notification settings - Fork 60
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
SignalManager: improve type safety and reduce boilerplate #1133
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,189 +10,128 @@ import { ContextMenuItemClickedSignalPayload } from './context-menu-item-clicked | |
import { RowSelectionsChangedSignalPayload } from './row-selections-changed-signal-payload'; | ||
import { ItemPropertiesSignalPayload } from './item-properties-signal-payload'; | ||
|
||
export declare interface SignalManager { | ||
fireTraceOpenedSignal(trace: Trace): void; | ||
fireTraceDeletedSignal(trace: Trace): void; | ||
fireExperimentExperimentSignal(experiment: Experiment): void; | ||
fireExperimentClosedSignal(experiment: Experiment): void; | ||
fireExperimentDeletedSignal(experiment: Experiment): void; | ||
fireExperimentSelectedSignal(experiment: Experiment | undefined): void; | ||
fireExperimentUpdatedSignal(experiment: Experiment): void; | ||
fireOpenedTracesChangedSignal(payload: OpenedTracesUpdatedSignalPayload): void; | ||
fireOutputAddedSignal(payload: OutputAddedSignalPayload): void; | ||
fireItemPropertiesSignalUpdated(payload: ItemPropertiesSignalPayload): void; | ||
fireThemeChangedSignal(theme: string): void; | ||
// TODO - Refactor or remove this signal. Similar signal to fireRequestSelectionRangeChange | ||
fireSelectionChangedSignal(payload: { [key: string]: string }): void; | ||
fireRowSelectionsChanged(payload: RowSelectionsChangedSignalPayload): void; | ||
fireCloseTraceViewerTabSignal(traceUUID: string): void; | ||
fireTraceViewerTabActivatedSignal(experiment: Experiment): void; | ||
fireUpdateZoomSignal(hasZoomedIn: boolean): void; | ||
fireResetZoomSignal(): void; | ||
fireMarkerCategoriesFetchedSignal(): void; | ||
fireMarkerSetsFetchedSignal(): void; | ||
fireMarkerCategoryClosedSignal(payload: { traceViewerId: string; markerCategory: string }): void; | ||
fireTraceServerStartedSignal(): void; | ||
fireUndoSignal(): void; | ||
fireRedoSignal(): void; | ||
fireOutputDataChanged(outputs: OutputDescriptor[]): void; | ||
fireOpenOverviewOutputSignal(traceId: string): void; | ||
export interface Signals { | ||
TRACE_OPENED: [trace: Trace]; | ||
TRACE_DELETED: [payload: { trace: Trace }]; | ||
EXPERIMENT_OPENED: [experiment: Experiment]; | ||
EXPERIMENT_CLOSED: [experiment: Experiment]; | ||
EXPERIMENT_DELETED: [experiment: Experiment]; | ||
EXPERIMENT_SELECTED: [experiment: Experiment | undefined]; | ||
EXPERIMENT_UPDATED: [experiment: Experiment]; | ||
OPENED_TRACES_UPDATED: [payload: OpenedTracesUpdatedSignalPayload]; | ||
AVAILABLE_OUTPUTS_CHANGED: void; | ||
OUTPUT_ADDED: [payload: OutputAddedSignalPayload]; | ||
ITEM_PROPERTIES_UPDATED: [payload: ItemPropertiesSignalPayload]; | ||
THEME_CHANGED: [theme: string]; | ||
SELECTION_CHANGED: [payload: { [key: string]: string }]; | ||
ROW_SELECTIONS_CHANGED: [payload: RowSelectionsChangedSignalPayload]; | ||
CLOSE_TRACEVIEWERTAB: [traceUUID: string]; | ||
TRACEVIEWERTAB_ACTIVATED: [experiment: Experiment]; | ||
UPDATE_ZOOM: [hasZoomedIn: boolean]; | ||
RESET_ZOOM: void; | ||
MARKER_CATEGORIES_FETCHED: void; | ||
MARKERSETS_FETCHED: void; | ||
MARKER_CATEGORY_CLOSED: [traceViewerId: string, markerCategory: string]; | ||
TRACE_SERVER_STARTED: void; | ||
UNDO: void; | ||
REDO: void; | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
firePinView(output: OutputDescriptor, payload?: any): void; | ||
PIN_VIEW: [output: OutputDescriptor, extra?: any]; | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
fireUnPinView(output: OutputDescriptor, payload?: any): void; | ||
fireOverviewOutputSelectedSignal(payload: { traceId: string; outputDescriptor: OutputDescriptor }): void; | ||
fireSaveAsCsv(payload: { traceId: string; data: string }): void; | ||
fireSelectionRangeUpdated(payload: TimeRangeUpdatePayload): void; | ||
fireViewRangeUpdated(payload: TimeRangeUpdatePayload): void; | ||
fireRequestSelectionRangeChange(payload: TimeRangeUpdatePayload): void; | ||
fireContributeContextMenu(payload: ContextMenuContributedSignalPayload): void; | ||
fireContextMenuItemClicked(payload: ContextMenuItemClickedSignalPayload): void; | ||
UNPIN_VIEW: [output: OutputDescriptor, extra?: any]; | ||
OPEN_OVERVIEW_OUTPUT: [traceId: string]; | ||
OVERVIEW_OUTPUT_SELECTED: [traceId: string, outputDescriptor: OutputDescriptor]; | ||
SAVE_AS_CSV: [traceId: string, data: string]; | ||
VIEW_RANGE_UPDATED: [payload: TimeRangeUpdatePayload]; | ||
SELECTION_RANGE_UPDATED: [payload: TimeRangeUpdatePayload]; | ||
REQUEST_SELECTION_RANGE_CHANGE: [payload: TimeRangeUpdatePayload]; | ||
OUTPUT_DATA_CHANGED: [descriptors: OutputDescriptor[]]; | ||
CONTRIBUTE_CONTEXT_MENU: [payload: ContextMenuContributedSignalPayload]; | ||
CONTEXT_MENU_ITEM_CLICKED: [payload: ContextMenuItemClickedSignalPayload]; | ||
} | ||
|
||
export const Signals = { | ||
TRACE_OPENED: 'trace opened', | ||
TRACE_DELETED: 'trace deleted', | ||
EXPERIMENT_OPENED: 'experiment opened', | ||
EXPERIMENT_CLOSED: 'experiment closed', | ||
EXPERIMENT_DELETED: 'experiment deleted', | ||
EXPERIMENT_SELECTED: 'experiment selected', | ||
EXPERIMENT_UPDATED: 'experiment updated', | ||
OPENED_TRACES_UPDATED: 'opened traces updated', | ||
AVAILABLE_OUTPUTS_CHANGED: 'available outputs changed', | ||
OUTPUT_ADDED: 'output added', | ||
ITEM_PROPERTIES_UPDATED: 'item properties updated', | ||
THEME_CHANGED: 'theme changed', | ||
SELECTION_CHANGED: 'selection changed', | ||
ROW_SELECTIONS_CHANGED: 'rows selected changed', | ||
CLOSE_TRACEVIEWERTAB: 'tab closed', | ||
TRACEVIEWERTAB_ACTIVATED: 'widget activated', | ||
UPDATE_ZOOM: 'update zoom', | ||
RESET_ZOOM: 'reset zoom', | ||
UNDO: 'undo', | ||
REDO: 'redo', | ||
MARKER_CATEGORIES_FETCHED: 'marker categories fetched', | ||
MARKERSETS_FETCHED: 'markersets fetched', | ||
MARKER_CATEGORY_CLOSED: 'marker category closed', | ||
TRACE_SERVER_STARTED: 'trace server started', | ||
PIN_VIEW: 'view pinned', | ||
UNPIN_VIEW: 'view unpinned', | ||
OPEN_OVERVIEW_OUTPUT: 'open overview output', | ||
OVERVIEW_OUTPUT_SELECTED: 'overview output selected', | ||
SAVE_AS_CSV: 'save as csv', | ||
VIEW_RANGE_UPDATED: 'view range updated', | ||
SELECTION_RANGE_UPDATED: 'selection range updated', | ||
REQUEST_SELECTION_RANGE_CHANGE: 'change selection range', | ||
OUTPUT_DATA_CHANGED: 'output data changed', | ||
CONTRIBUTE_CONTEXT_MENU: 'contribute context menu', | ||
CONTEXT_MENU_ITEM_CLICKED: 'context menu item clicked' | ||
}; | ||
type SignalType = keyof Signals; | ||
type SignalArgs<T> = T extends void ? [] : T; | ||
|
||
export class SignalManager extends EventEmitter implements SignalManager { | ||
fireTraceOpenedSignal(trace: Trace): void { | ||
this.emit(Signals.TRACE_OPENED, trace); | ||
} | ||
fireTraceDeletedSignal(trace: Trace): void { | ||
this.emit(Signals.TRACE_DELETED, { trace }); | ||
} | ||
fireExperimentOpenedSignal(experiment: Experiment): void { | ||
this.emit(Signals.EXPERIMENT_OPENED, experiment); | ||
} | ||
fireExperimentClosedSignal(experiment: Experiment): void { | ||
this.emit(Signals.EXPERIMENT_CLOSED, experiment); | ||
} | ||
fireExperimentDeletedSignal(experiment: Experiment): void { | ||
this.emit(Signals.EXPERIMENT_DELETED, experiment); | ||
} | ||
fireExperimentSelectedSignal(experiment: Experiment | undefined): void { | ||
this.emit(Signals.EXPERIMENT_SELECTED, experiment); | ||
} | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
fireRowSelectionsChanged(payload: RowSelectionsChangedSignalPayload): void { | ||
this.emit(Signals.ROW_SELECTIONS_CHANGED, payload); | ||
} | ||
fireExperimentUpdatedSignal(experiment: Experiment): void { | ||
this.emit(Signals.EXPERIMENT_UPDATED, experiment); | ||
} | ||
fireOpenedTracesChangedSignal(payload: OpenedTracesUpdatedSignalPayload): void { | ||
this.emit(Signals.OPENED_TRACES_UPDATED, payload); | ||
} | ||
fireOutputAddedSignal(payload: OutputAddedSignalPayload): void { | ||
this.emit(Signals.OUTPUT_ADDED, payload); | ||
} | ||
fireItemPropertiesSignalUpdated(payload: ItemPropertiesSignalPayload): void { | ||
this.emit(Signals.ITEM_PROPERTIES_UPDATED, payload); | ||
} | ||
fireThemeChangedSignal(theme: string): void { | ||
this.emit(Signals.THEME_CHANGED, theme); | ||
} | ||
fireSelectionChangedSignal(payload: { [key: string]: string }): void { | ||
this.emit(Signals.SELECTION_CHANGED, payload); | ||
} | ||
fireCloseTraceViewerTabSignal(traceUUID: string): void { | ||
this.emit(Signals.CLOSE_TRACEVIEWERTAB, traceUUID); | ||
} | ||
fireTraceViewerTabActivatedSignal(experiment: Experiment): void { | ||
this.emit(Signals.TRACEVIEWERTAB_ACTIVATED, experiment); | ||
} | ||
fireUpdateZoomSignal(hasZoomedIn: boolean): void { | ||
this.emit(Signals.UPDATE_ZOOM, hasZoomedIn); | ||
} | ||
fireResetZoomSignal(): void { | ||
this.emit(Signals.RESET_ZOOM); | ||
export class SignalManager extends EventEmitter { | ||
/** | ||
* Registers an event handler for a specific signal type. | ||
* Provides type-safe event registration with correct payload types for each signal. | ||
* | ||
* @template K - The signal type (key of Signals interface) | ||
* @param event - The event name to listen for | ||
* @param listener - The callback function to execute when the event occurs | ||
* Type of arguments is automatically inferred from Signals interface | ||
* @returns The signal manager instance for chaining | ||
* | ||
* @example | ||
* // Single argument event | ||
* signalManager().on('THEME_CHANGED', (theme: string) => { | ||
* console.log(`Theme changed to: ${theme}`); | ||
* }); | ||
* | ||
* // Tuple argument event | ||
* signalManager().on('PIN_VIEW', (output: OutputDescriptor, extra?: any) => { | ||
* console.log(`Pinning view for output: ${output.id}`); | ||
* }); | ||
*/ | ||
on<K extends SignalType>( | ||
event: K, | ||
listener: ( | ||
...args: SignalArgs<Signals[K]> extends [] ? [] : [...SignalArgs<Signals[K]>] | ||
) => void | Promise<void> | ||
): this { | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
return super.on(event, listener as (...args: any[]) => void | Promise<void>); | ||
} | ||
fireMarkerCategoriesFetchedSignal(): void { | ||
this.emit(Signals.MARKER_CATEGORIES_FETCHED); | ||
} | ||
fireMarkerSetsFetchedSignal(): void { | ||
this.emit(Signals.MARKERSETS_FETCHED); | ||
} | ||
fireMarkerCategoryClosedSignal(payload: { traceViewerId: string; markerCategory: string }): void { | ||
this.emit(Signals.MARKER_CATEGORY_CLOSED, payload); | ||
} | ||
fireTraceServerStartedSignal(): void { | ||
this.emit(Signals.TRACE_SERVER_STARTED); | ||
} | ||
fireUndoSignal(): void { | ||
this.emit(Signals.UNDO); | ||
} | ||
fireRedoSignal(): void { | ||
this.emit(Signals.REDO); | ||
} | ||
fireOutputDataChanged(outputs: OutputDescriptor[]): void { | ||
this.emit(Signals.OUTPUT_DATA_CHANGED, outputs); | ||
} | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types | ||
firePinView(output: OutputDescriptor, payload?: any): void { | ||
this.emit(Signals.PIN_VIEW, output, payload); | ||
} | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types | ||
fireUnPinView(output: OutputDescriptor, payload?: any): void { | ||
this.emit(Signals.UNPIN_VIEW, output, payload); | ||
} | ||
fireOpenOverviewOutputSignal(traceId: string): void { | ||
this.emit(Signals.OPEN_OVERVIEW_OUTPUT, traceId); | ||
} | ||
fireOverviewOutputSelectedSignal(payload: { traceId: string; outputDescriptor: OutputDescriptor }): void { | ||
this.emit(Signals.OVERVIEW_OUTPUT_SELECTED, payload); | ||
} | ||
fireSaveAsCsv(payload: { traceId: string; data: string }): void { | ||
this.emit(Signals.SAVE_AS_CSV, payload); | ||
} | ||
fireViewRangeUpdated(payload: TimeRangeUpdatePayload): void { | ||
this.emit(Signals.VIEW_RANGE_UPDATED, payload); | ||
} | ||
fireSelectionRangeUpdated(payload: TimeRangeUpdatePayload): void { | ||
this.emit(Signals.SELECTION_RANGE_UPDATED, payload); | ||
} | ||
fireRequestSelectionRangeChange(payload: TimeRangeUpdatePayload): void { | ||
this.emit(Signals.REQUEST_SELECTION_RANGE_CHANGE, payload); | ||
} | ||
fireContributeContextMenu(payload: ContextMenuContributedSignalPayload): void { | ||
this.emit(Signals.CONTRIBUTE_CONTEXT_MENU, payload); | ||
|
||
/** | ||
* Removes an event handler for a specific signal type. | ||
* Ensures type safety by requiring the listener signature to match the signal type. | ||
* | ||
* @template K - The signal type (key of Signals interface) | ||
* @param event - The event name to remove the listener from | ||
* @param listener - The callback function to remove | ||
* @returns The signal manager instance for chaining | ||
* | ||
* @example | ||
* const themeHandler = (theme: string) => console.log(theme); | ||
* signalManager().off('THEME_CHANGED', themeHandler); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that a String has to be passed manually instead of a String constant as before (Signals.THEME_CHANGED) is more error-prone, because it's not caught at build time. Think we should have something similar as before. |
||
*/ | ||
off<K extends SignalType>( | ||
event: K, | ||
listener: ( | ||
...args: SignalArgs<Signals[K]> extends [] ? [] : [...SignalArgs<Signals[K]>] | ||
) => void | Promise<void> | ||
): this { | ||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
return super.off(event, listener as (...args: any[]) => void | Promise<void>); | ||
} | ||
fireContextMenuItemClicked(payload: ContextMenuItemClickedSignalPayload): void { | ||
this.emit(Signals.CONTEXT_MENU_ITEM_CLICKED, payload); | ||
|
||
/** | ||
* Emits a signal with type-safe arguments based on the signal type. | ||
* Arguments are automatically validated against the Signals interface. | ||
* | ||
* @template K - The signal type (key of Signals interface) | ||
* @param event - The event name to emit | ||
* @param args - The arguments to pass to listeners, type checked against Signals interface | ||
* @returns true if the event had listeners, false otherwise | ||
* | ||
* @example | ||
* // Single argument emission | ||
* signalManager().emit('THEME_CHANGED', 'dark'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that a String has to be passed manually instead of a String constant as before (Signals.THEME_CHANGED) is more error-prone, because it's not caught at build time. Think we should have something similar as before. |
||
* | ||
* // Tuple argument emission | ||
* signalManager().emit('MARKER_CATEGORY_CLOSED', 'viewer1', 'category1'); | ||
* | ||
* // Void event emission | ||
* signalManager().emit('RESET_ZOOM'); | ||
*/ | ||
emit<K extends SignalType>( | ||
event: K, | ||
...args: SignalArgs<Signals[K]> extends [] ? [] : [...SignalArgs<Signals[K]>] | ||
): boolean { | ||
return super.emit(event, ...args); | ||
} | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that a String has to be passed manually instead of a String constant as before (Signals.THEME_CHANGED) is more error-prone, because it's not caught at build time. Think we should have something similar as before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current setup is actually type-safe! The
SignalType = keyof Signals
type means TypeScript will only accept exact string matches of the events defined in ourSignals
interface. You can't pass an arbitrary string - TypeScript will catch any typos or invalid events at compile time:Plus, we get the bonus of TypeScript automatically inferring the correct parameter types for our listeners. So we're getting all the safety of constants with better ergonomics.
Here's a screengrab of how this looks in vscode:
Screencast.from.10-25-2024.09.55.57.AM.webm