SignalManager: improve type safety and reduce boilerplate #1133

williamsyang-work · 2024-10-22T19:14:21Z

⚠️ IMPORTANT: Dependency Warning ⚠️

This PR introduces type changes that will affect the vscode-trace-extension repository.
DO NOT MERGE until maintainers have reviewed and approved the proposed changes.
A corresponding PR will need to be created in vscode-trace-extension to handle these type updates.

👉 Maintainers: Please review and comment on the proposed type system before work begins on the extension repository.

Improve SignalManager Type Safety and Developer Experience

Overview

This PR refactors the SignalManager to provide better TypeScript support and improved developer experience. The changes focus on making the event system more type-safe while reducing boilerplate code.

Key Improvements

💡 Better Type Inference

// Before - No type inference on payload
signalManager.on('THEME_CHANGED', (theme) => {
    console.log(theme.toUpperCase()); // TS doesn't know theme is a string
});

// After - Full type inference
signalManager.on('THEME_CHANGED', (theme) => {
    console.log(theme.toUpperCase()); // TS knows theme is a string
});

🛡️ Compile-Time Type Safety

// Before - TypeScript wouldn't catch this error
signalManager.emit('THEME_CHANGED', 123); // Should be string

// After - TypeScript catches invalid payload types
signalManager.emit('THEME_CHANGED', 123); // Type error!

📝 Better IDE Support

Autocomplete for event names
Parameter type hints for event handlers
Inline documentation for all methods

🧹 Reduced Boilerplate

// Before - Required separate fire* method for each event
class SignalManager {
    fireThemeChangedSignal(theme: string): void {
        this.emit(Signals.THEME_CHANGED, theme);
    }
    fireTraceOpenedSignal(trace: Trace): void {
        this.emit(Signals.TRACE_OPENED, trace);
    }
    // ... many more fire* methods
}

// After - Direct emit with type safety
signalManager.emit('THEME_CHANGED', theme);
signalManager.emit('TRACE_OPENED', trace);

Implementation Details

Consolidated signal types into a single Signals interface
Added generic type constraints for event handlers
Enhanced JSDoc documentation for core methods
Maintained backward compatibility with existing event system

bhufmann

Thanks for the proposal. It looks like a simplification. I had a first glance over the changes and have a general comment using Strings. Please have look.

packages/base/src/signals/signal-manager.ts

bhufmann · 2024-10-24T17:24:19Z

packages/base/src/signals/signal-manager.ts

+     *
+     * @example
+     * // Single argument event
+     * signalManager().on('THEME_CHANGED', (theme: string) => {


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.

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

Thanks for the answers. I'll review it.

packages/base/src/signals/signal-manager.ts

bhufmann · 2024-10-28T20:00:09Z

FYI @ngondalia. Please let us know if you have any concerns. I'll update the vscode-trace-extension accordingly once we have decided to merge this.

bhufmann · 2024-10-28T20:11:11Z

@williamsyang-work I want to understand the impact to the vscode-trace-extension. I'm currently struggling to make it compile for the external-api (https://github.com/eclipse-cdt-cloud/vscode-trace-extension/blob/e1bfcee1493621b060889a0e79c993c0748931d5/vscode-trace-extension/src/external-api/external-api.ts#L78).

I get the following error

Argument of type 'string | symbol' is not assignable to parameter of type 'keyof Signals'.
  Type 'string' is not assignable to type 'keyof Signals'.ts(2345)

Any suggestions?

bhufmann · 2024-10-29T13:09:11Z

@williamsyang-work I want to understand the impact to the vscode-trace-extension. I'm currently struggling to make it compile for the external-api (https://github.com/eclipse-cdt-cloud/vscode-trace-extension/blob/e1bfcee1493621b060889a0e79c993c0748931d5/vscode-trace-extension/src/external-api/external-api.ts#L78).

I get the following error
Argument of type 'string | symbol' is not assignable to parameter of type 'keyof Signals'.
  Type 'string' is not assignable to type 'keyof Signals'.ts(2345)
Any suggestions?

I think we could change the API

 offSignalManagerSignal(event: string | symbol, listener: (...args: unknown[]) => void): void {
        signalManager().off(event, listener);
    },

to the following

offSignalManagerSignal(event: keyof Signals, listener: (...args: unknown[]) => void): void {
        signalManager().off(event, listener);
    },

This will break downstream projects. @ngondalia please let us know if that is ok with you.

williamsyang-work · 2024-10-29T18:15:43Z

@williamsyang-work I want to understand the impact to the vscode-trace-extension. I'm currently struggling to make it compile for the external-api (https://github.com/eclipse-cdt-cloud/vscode-trace-extension/blob/e1bfcee1493621b060889a0e79c993c0748931d5/vscode-trace-extension/src/external-api/external-api.ts#L78).
I get the following error
Argument of type 'string | symbol' is not assignable to parameter of type 'keyof Signals'.
  Type 'string' is not assignable to type 'keyof Signals'.ts(2345)
Any suggestions?
I think we could change the API
 offSignalManagerSignal(event: string | symbol, listener: (...args: unknown[]) => void): void {
        signalManager().off(event, listener);
    },
to the following
offSignalManagerSignal(event: keyof Signals, listener: (...args: unknown[]) => void): void {
        signalManager().off(event, listener);
    },
This will break downstream projects. @ngondalia please let us know if that is ok with you.

@bhufmann I haven't looked into the changes needed for vscode-trace-extension. I wanted to get some positive feedback on this change before investigating what is needed for that repo. If you think this is a good idea I can start investigating the changes required for this in vscode.

I think that the change you suggested makes sense.

ngondalia · 2024-10-29T18:58:16Z

@williamsyang-work I want to understand the impact to the vscode-trace-extension. I'm currently struggling to make it compile for the external-api (https://github.com/eclipse-cdt-cloud/vscode-trace-extension/blob/e1bfcee1493621b060889a0e79c993c0748931d5/vscode-trace-extension/src/external-api/external-api.ts#L78).
I get the following error
Argument of type 'string | symbol' is not assignable to parameter of type 'keyof Signals'.
  Type 'string' is not assignable to type 'keyof Signals'.ts(2345)
Any suggestions?
I think we could change the API
 offSignalManagerSignal(event: string | symbol, listener: (...args: unknown[]) => void): void {
        signalManager().off(event, listener);
    },
to the following
offSignalManagerSignal(event: keyof Signals, listener: (...args: unknown[]) => void): void {
        signalManager().off(event, listener);
    },
This will break downstream projects. @ngondalia please let us know if that is ok with you.

This will require a bit of refactoring on our end, but i think it is a good improvement. I am ok with this. :)

williamsyang-work · 2024-10-29T20:57:06Z

Here is a draft PR I made with changes to the vscode-trace-extension. @ngondalia I changed the ExternalAPI typing so that the type enforcement will work in downstream projects.

eclipse-cdt-cloud/vscode-trace-extension#277

bhufmann

Code looks good to me. I ran it in Theia and with the vscode extension update (eclipse-cdt-cloud/vscode-trace-extension#277) and it works.

BTW, the license error is taken care of and we can still proceed with merging this PR:
https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/16973

bhufmann · 2024-10-31T18:19:05Z

@ngondalia would you be able to do another review for this one? If not, please let me know and then I'll ask another committer.

ngondalia · 2024-11-01T15:05:05Z

@ngondalia would you be able to do another review for this one? If not, please let me know and then I'll ask another committer.

Yeah, i can review it - Give me until the end of the day (I would like to try it out with both updates)

ngondalia · 2024-11-01T15:58:20Z

;

@ngondalia would you be able to do another review for this one? If not, please let me know and then I'll ask another committer.

Yeah, i can review it - Give me until the end of the day (I would like to try it out with both updates)

There are some merge conflicts - otherwise LGTM

williamsyang-work · 2024-11-01T18:55:46Z

There are some merge conflicts - otherwise LGTM

@ngondalia
Resolved

bhufmann

@williamsyang-work there are linter errors. Please run yarn format:write to fix them and then re-submit the changes.

Also it needs a rebase after recent merges.

PatrickTasse

I too was worried about not having an enum or constants for the signal names, but the auto-complete does give the list of possible signals.

PatrickTasse · 2024-11-13T16:18:32Z

packages/react-components/src/trace-explorer/trace-explorer-views-widget.tsx

-        signalManager().on(Signals.EXPERIMENT_SELECTED, this._onExperimentSelected);
-        signalManager().on(Signals.EXPERIMENT_CLOSED, this._onExperimentClosed);
+        signalManager().on("EXPERIMENT_SELECTED", this._onExperimentSelected);
+        signalManager().on("EXPERIMENT_CLOSED", this._onExperimentClosed);


Single quotes

Done. Sorry for the delay. @bhufmann

No problem. I'll re-review.

- Replace individual fire* methods with strongly-typed emit interface - Consolidate signal payload types into single Signals interface - Add type-safe event handler registration with overloaded on/off methods - Remove redundant method declarations in favor of TypeScript inference - Maintain full backward compatibility with existing event system The new implementation provides compile-time type checking for event payloads while significantly reducing code volume and maintenance burden. Signed-off-by: Will Yang <[email protected]>

Dismissing review. I verified that Patrick's comments are addressed. Patrick is currently not available for another review.

bhufmann

It looks good to me. Thanks for the contribution.

MatthewKhouzam

I like this approach. I am approving but make sure to merge only when ready.

williamsyang-work force-pushed the signal-type-enforcement branch 2 times, most recently from e381965 to a2e3573 Compare October 22, 2024 19:39

williamsyang-work marked this pull request as ready for review October 22, 2024 19:53

williamsyang-work marked this pull request as draft October 22, 2024 20:00

williamsyang-work force-pushed the signal-type-enforcement branch from a2e3573 to bfa8d0c Compare October 22, 2024 20:08

williamsyang-work marked this pull request as ready for review October 22, 2024 22:27

williamsyang-work force-pushed the signal-type-enforcement branch 4 times, most recently from 2720135 to 5b2699a Compare October 23, 2024 18:02

williamsyang-work changed the title ~~refactor(SignalManager): improve type safety and reduce boilerplate~~ SignalManager: improve type safety and reduce boilerplate Oct 23, 2024

williamsyang-work force-pushed the signal-type-enforcement branch from 5b2699a to 3441fdc Compare October 23, 2024 22:39

bhufmann requested changes Oct 24, 2024

View reviewed changes

williamsyang-work force-pushed the signal-type-enforcement branch from 3441fdc to aca8607 Compare October 29, 2024 20:58

bhufmann previously approved these changes Oct 31, 2024

View reviewed changes

williamsyang-work dismissed bhufmann’s stale review via e7de326 November 1, 2024 18:55

williamsyang-work force-pushed the signal-type-enforcement branch from aca8607 to e7de326 Compare November 1, 2024 18:55

bhufmann requested changes Nov 4, 2024

View reviewed changes

PatrickTasse previously requested changes Nov 13, 2024

View reviewed changes

williamsyang-work force-pushed the signal-type-enforcement branch from e7de326 to 6d29d50 Compare November 20, 2024 22:50

williamsyang-work force-pushed the signal-type-enforcement branch from 6d29d50 to 1e8c9fc Compare November 20, 2024 22:55

bhufmann approved these changes Nov 26, 2024

View reviewed changes

MatthewKhouzam approved these changes Nov 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SignalManager: improve type safety and reduce boilerplate #1133

SignalManager: improve type safety and reduce boilerplate #1133

williamsyang-work commented Oct 22, 2024 •

edited

Loading

bhufmann left a comment

bhufmann Oct 24, 2024

williamsyang-work Oct 25, 2024 •

edited

Loading

bhufmann Oct 28, 2024

bhufmann commented Oct 28, 2024 •

edited

Loading

bhufmann commented Oct 28, 2024 •

edited

Loading

bhufmann commented Oct 29, 2024

williamsyang-work commented Oct 29, 2024 •

edited

Loading

ngondalia commented Oct 29, 2024

williamsyang-work commented Oct 29, 2024 •

edited

Loading

bhufmann left a comment •

edited

Loading

bhufmann commented Oct 31, 2024

ngondalia commented Nov 1, 2024

ngondalia commented Nov 1, 2024

williamsyang-work commented Nov 1, 2024 •

edited

Loading

bhufmann left a comment •

edited

Loading

PatrickTasse left a comment

PatrickTasse Nov 13, 2024

williamsyang-work Nov 20, 2024

bhufmann Nov 26, 2024

bhufmann left a comment

MatthewKhouzam left a comment

SignalManager: improve type safety and reduce boilerplate #1133

Are you sure you want to change the base?

SignalManager: improve type safety and reduce boilerplate #1133

Conversation

williamsyang-work commented Oct 22, 2024 • edited Loading

Improve SignalManager Type Safety and Developer Experience

Overview

Key Improvements

💡 Better Type Inference

🛡️ Compile-Time Type Safety

📝 Better IDE Support

🧹 Reduced Boilerplate

Implementation Details

bhufmann left a comment

Choose a reason for hiding this comment

bhufmann Oct 24, 2024

Choose a reason for hiding this comment

williamsyang-work Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

bhufmann Oct 28, 2024

Choose a reason for hiding this comment

bhufmann commented Oct 28, 2024 • edited Loading

bhufmann commented Oct 28, 2024 • edited Loading

bhufmann commented Oct 29, 2024

williamsyang-work commented Oct 29, 2024 • edited Loading

ngondalia commented Oct 29, 2024

williamsyang-work commented Oct 29, 2024 • edited Loading

bhufmann left a comment • edited Loading

Choose a reason for hiding this comment

bhufmann commented Oct 31, 2024

ngondalia commented Nov 1, 2024

ngondalia commented Nov 1, 2024

williamsyang-work commented Nov 1, 2024 • edited Loading

bhufmann left a comment • edited Loading

Choose a reason for hiding this comment

PatrickTasse left a comment

Choose a reason for hiding this comment

PatrickTasse Nov 13, 2024

Choose a reason for hiding this comment

williamsyang-work Nov 20, 2024

Choose a reason for hiding this comment

bhufmann Nov 26, 2024

Choose a reason for hiding this comment

bhufmann left a comment

Choose a reason for hiding this comment

MatthewKhouzam left a comment

Choose a reason for hiding this comment

williamsyang-work commented Oct 22, 2024 •

edited

Loading

williamsyang-work Oct 25, 2024 •

edited

Loading

bhufmann commented Oct 28, 2024 •

edited

Loading

bhufmann commented Oct 28, 2024 •

edited

Loading

williamsyang-work commented Oct 29, 2024 •

edited

Loading

williamsyang-work commented Oct 29, 2024 •

edited

Loading

bhufmann left a comment •

edited

Loading

williamsyang-work commented Nov 1, 2024 •

edited

Loading

bhufmann left a comment •

edited

Loading