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

Fix events.subscribe types: returns either an async or promise function #563

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

Conversation

filmaj
Copy link
Member

@filmaj filmaj commented Aug 17, 2024

Returning unknown from events.subscribe, as the types do today, sucks if you're writing tests:

Screenshot 2024-08-17 at 17 22 08

This PR attempts to address this by tweaking the return type of subscribe, which is conditional based on what kind of event handler function you pass into subscribe:

  1. If you pass an async function, subscribe returns an async function.
  2. If you pass it a regular function that accepts two parameters, the second parameter being a callback, it returns a regular function that takes three parameters, the last being a callback too.

With these changes, now my tests are tidier with less IDE complaints. With an async handler the result looks like:

Screenshot 2024-08-17 at 17 19 33

.. and using a callback interface, too:

Screenshot 2024-08-17 at 17 18 52

No matter which style of handler you pass, at least in the context of my tests, now my IDE knows that I am dealing with functions - even the right kinds to boot!

@filmaj filmaj requested a review from tbeseda August 17, 2024 21:23
@filmaj filmaj self-assigned this Aug 17, 2024
…wn, but a function returning a promise or not, depending on whether an async or callback function was provided to subscribe

dont care about args to callbacks
@filmaj filmaj force-pushed the event-subscribe-types branch from 246fb91 to 08ccc44 Compare August 17, 2024 21:25
Copy link
Member

@tbeseda tbeseda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better. Thanks 🤘

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

Successfully merging this pull request may close these issues.

2 participants