-
Notifications
You must be signed in to change notification settings - Fork 30
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
Async / Promise support? #11
Comments
do you have an example? i'm curious
|
Sure! I have the following use case:
Performing the HTTP request in step 6 is an asynchronous call. Because it is important that the library code doesn't continue executing before the Axios HTTP client is fully configured (with middleware that sets a bearer token), here's what I need to do from my library's point of view (pseudo code): var axiosConfig = {};
await eventBus.emit('configure-http-client', this, axiosConfig);
this.axiosClient = new axiosClient(axiosConfig); For now I took your code and adapted it in its entirety so that for me there's no longer an option to invoke the event bus synchronously; I made the That's fine for my purposes, but would break your users. So maybe introducing an |
Hi @sfmskywalker, I think it is a good idea to allow a user to add an async listener and be able to wait for the response when emitting the event. I kinda like your idea to add an The only problem is that this functionality is not supported on IE 11 and this library has been made in such a way and code that it supports old browsers. But I was asking myself if I should not rewrite a 2.0 version of the library in a much modern code. So, we'd be able to add such functionalities. |
Great! IMHO a 2.0 for the modern browser makes much sense. Users that need to support legacy browsers can of course keep using 1.x. Should there be great demand for a 1.x version that also supports async, we could probably implement it using promises and If you want I'd be happy to contribute a 2.0 branch taking your code and rewrite it a little bit. But up to you. I can imagine it might be fun to do :) |
Indeed, I think we can ignore the 1.x branch. It can be added later if someone needs it, but I doubt it. A new 2.0 version is the best option. But my plans was, before refactoring, writing some tests to be sure to not break anything (even for a 2.0 release). It can take time to do tests and refactor. But I'll think about it in the next few weeks. |
Makes perfect sense. |
Hi @bcerati, hope all is well. Just checking in with you to see if you were able to take any stab at writing the tests. No worries if that's not the case, I know everyone is busy. I'm currently working on a new version of my project during which I'm porting over some code from the previous version. So I figured, this may be a great opportunity to use a 2.0 version of js-event-bus 😃 |
@sfmskywalker Can I suggest another approach to this problem that does not require API changes? What if you emit 2 events: |
@uglow yes that'll do the trick ;) I have rewritten all the lib using last JS features and typescript by default. It's not released yet. It's currently in the main branch. I'll add the feature requested in this issue in the release. |
Hi @bcerati , can this be considered closed ? |
Hi! I've been using your awesome library for a long time now and I love its ease of use!
Today I found the need to be able to have an async event handler. However, the event bus doesn't wait for the handler to complete of course since it doesn't handle promises.
Do you think it would be a straightforward-enough addition to check for promises and await them? If yes, I would be happy to provide a PR. Just need to know if you think it makes sense to have it.
The text was updated successfully, but these errors were encountered: