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.

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

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

[TS] Return unsub() from on() #138

wants to merge 1 commit into from

Conversation

developit
Copy link
Owner

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.

@@ -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);
Copy link
Owner Author

@developit developit Jun 23, 2021

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().

@github-actions
Copy link

Size Change: +44 B (+5%) 🔍

Total Size: 1 kB

Filename Size Change
dist/mitt.es.js 231 B +12 B (+5%) 🔍
dist/mitt.js 230 B +12 B (+6%) 🔍
dist/mitt.modern.js 231 B +8 B (+4%)
dist/mitt.umd.js 308 B +12 B (+4%)

compressed-size-action

@y-nk
Copy link

y-nk commented Jul 13, 2021

@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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants