Skip to content
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.

Sign up for GitHub

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

Jump to bottom

SignalManager: improve type safety and reduce boilerplate #1133

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

SignalManager: improve type safety and reduce boilerplate #1133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions packages/base/src/experiment-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { OutputDescriptor } from 'tsp-typescript-client/lib/models/output-descri
import { Experiment } from 'tsp-typescript-client/lib/models/experiment';
import { TraceManager } from './trace-manager';
import { TspClientResponse } from 'tsp-typescript-client/lib/protocol/tsp-client-response';
import { signalManager, Signals } from './signals/signal-manager';
import { signalManager } from './signals/signal-manager';

export class ExperimentManager {
private fOpenExperiments: Map<string, Experiment> = new Map();
Expand All @@ -15,9 +15,7 @@ export class ExperimentManager {
constructor(tspClient: ITspClient, traceManager: TraceManager) {
this.fTspClient = tspClient;
this.fTraceManager = traceManager;
signalManager().on(Signals.EXPERIMENT_DELETED, (experiment: Experiment) =>
this.onExperimentDeleted(experiment)
);
signalManager().on('EXPERIMENT_DELETED', (experiment: Experiment) => this.onExperimentDeleted(experiment));
}

/**
Expand Down Expand Up @@ -99,7 +97,7 @@ export class ExperimentManager {
const experiment = experimentResponse.getModel();
if (experimentResponse.isOk() && experiment) {
this.addExperiment(experiment);
signalManager().fireExperimentOpenedSignal(experiment);
signalManager().emit('EXPERIMENT_OPENED', experiment);
return experiment;
}
// TODO Handle any other experiment open errors
Expand Down Expand Up @@ -131,7 +129,7 @@ export class ExperimentManager {
await this.fTspClient.deleteExperiment(experimentUUID);
const deletedExperiment = this.removeExperiment(experimentUUID);
if (deletedExperiment) {
signalManager().fireExperimentDeletedSignal(deletedExperiment);
signalManager().emit('EXPERIMENT_DELETED', deletedExperiment);
}
}
}
Expand Down
291 changes: 115 additions & 176 deletions packages/base/src/signals/signal-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Copy link
Collaborator

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.

Copy link
Contributor Author

@williamsyang-work williamsyang-work Oct 25, 2024 •
edited
Loading

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 our Signals interface. You can't pass an arbitrary string - TypeScript will catch any typos or invalid events at compile time:

signalManager.on('THEME_CHANGED', ...) // ✅ Works fine
signalManager.on('INVALID_EVENT', ...) // ❌ Compile error
signalManager.on('theme_changed', ...) // ❌ Compile error

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

* 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);
Copy link
Collaborator

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.

*/
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');
Copy link
Collaborator

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.

*
* // 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);
}
}

Expand Down
4 changes: 2 additions & 2 deletions packages/base/src/trace-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ export class TraceManager {
const trace = traceResponse.getModel();
if (traceResponse.isOk() && trace) {
this.addTrace(trace);
signalManager().fireTraceOpenedSignal(trace);
signalManager().emit('TRACE_OPENED', trace);
return trace;
}
// TODO Handle trace open errors
Expand Down Expand Up @@ -123,7 +123,7 @@ export class TraceManager {
if (deleteResponse.getStatusCode() !== 409) {
const deletedTrace = this.removeTrace(traceUUID);
if (deletedTrace) {
signalManager().fireTraceDeletedSignal(deletedTrace);
signalManager().emit('TRACE_DELETED', { trace: deletedTrace });
}
}
}
Expand Down
8 changes: 4 additions & 4 deletions packages/react-components/src/components/abstract-output-component.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ export abstract class AbstractOutputComponent<

private closeComponent() {
if (this.props.pinned) {
signalManager().fireUnPinView(this.props.outputDescriptor);
signalManager().emit('UNPIN_VIEW', this.props.outputDescriptor);
}
this.props.onOutputRemove(this.props.outputDescriptor.id);
}
Expand Down Expand Up @@ -271,15 +271,15 @@ export abstract class AbstractOutputComponent<

// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types
protected pinView(payload?: any): void {
signalManager().firePinView(this.props.outputDescriptor, payload);
signalManager().emit('PIN_VIEW', this.props.outputDescriptor, payload);
}

// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/explicit-module-boundary-types
protected unPinView(payload?: any): void {
if (payload) {
signalManager().fireUnPinView(this.props.outputDescriptor, payload);
signalManager().emit('UNPIN_VIEW', this.props.outputDescriptor, payload);
} else {
signalManager().fireUnPinView(this.props.outputDescriptor);
signalManager().emit('UNPIN_VIEW', this.props.outputDescriptor);
}
}

Expand Down
Loading
Loading