-
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?
SignalManager: improve type safety and reduce boilerplate #1133
Conversation
e381965
to
a2e3573
Compare
a2e3573
to
bfa8d0c
Compare
2720135
to
5b2699a
Compare
- 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]>
5b2699a
to
3441fdc
Compare
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.
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.
* | ||
* @example | ||
* // Single argument emission | ||
* signalManager().emit('THEME_CHANGED', 'dark'); |
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.
* | ||
* @example | ||
* // Single argument event | ||
* signalManager().on('THEME_CHANGED', (theme: string) => { |
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 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
* | ||
* @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 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.
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
🛡️ Compile-Time Type Safety
📝 Better IDE Support
🧹 Reduced Boilerplate
Implementation Details
Signals
interface