-
Notifications
You must be signed in to change notification settings - Fork 446
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
[TS] Return unsub() from on() #138
base: main
Are you sure you want to change the base?
Conversation
@@ -75,13 +76,13 @@ export default function mitt<Events extends Record<EventType, unknown>>( | |||
* @memberOf mitt | |||
*/ | |||
off<Key extends keyof Events>(type: Key, handler?: GenericEventHandler) { | |||
const handlers: Array<GenericEventHandler> | undefined = all!.get(type); | |||
let handlers: Array<GenericEventHandler> | undefined = all!.get(type); |
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.
Note: this is a non-functional change - it just slightly improves compression gains by making the structure of off()
match that of on()
.
Size Change: +44 B (+5%) 🔍 Total Size: 1 kB
|
@developit i think the feature would find itself very convenient if you consider: 1. wrapping many events in some qualified functions:function someSpecificFunction() {
const handler = () => console.log(arguments)
const unsubs = ['event1', 'event2', 'event3'].map(ev => {
mitt.on(ev, handler)
return () => mitt.off(ev, handler)
})
return () => unsubs.forEach(fn => fn())
} which, with this PR could be made more concise like: function someSpecificFunction() {
const handler = () => console.log(arguments)
const unsubs = [
mitt.on('event1', handler),
mitt.on('event2', handler),
mitt.on('event3', handler),
]
return () => unsubs.forEach(fn => fn())
} 2. use in modern frontend libraries such as React:A typical use could be: useEffect(() => {
const listener = mitt.on('event', setStateSomething)
return listener
}, [someDep]) which, with this PR could be made more concise like: useEffect(
() => mitt.on('event', setStateSomething),
[someDep],
) In the end people could just also have a workaround such as: const on = mitt.on
mitt.on = (ev, fn) => on(ev, fn) ?? () => mitt.off(ev, fn) ...but it's such a common use nowadays that it just doesn't make sense not to have it. (i personally look forward to this PR to start using mitt into my company's sdk) |
@developit hi. this looks what I am looking for. are you still not considering merging it? |
This is another take on the proposed behavior from #42, where
on()
returns a function that can be called to remove the added listener.My concerns remain, however, and I don't think this should be landed. Any implementation that provides this convenience makes it easier to create memory leaks, because there are now two separate owners of a given handler reference - Mitt itself, and the app code that added the listener.