EventSource: promises, priorities and execution breaks

[Aiosa]

Adding support for type detection: async function and promise.
New features in EventSource:

• ability to wait for promise-returning functions if specified (still documented in the original PR)
• ability to prioritize calls
• ability to exit raised event for all unfinished callbacks (still documented in the original PR)

[Aiosa]

I forgot to remove package-lock.json from the commit, however, only thing updated is the version string which was incorrect in the source anyway (releases now contain 4.0.0, the file has 3.1.0). Not sure if I can leave it there or whether I should try to remove it..

[Aiosa]

I guess a simple tutorial page on EventSource could be also useful... I was also thinking about splitting raiseEvent to two implementations (or reading a flag) that would enable breaking explicitly since we don't really want let the user to exit build-in events early. Or do we? It is a new level of flexibility indeed.

I even more think about splitting raiseEvent for two - synced and non-synced versions. Since one returns undefined and other promise.

Edit: truth is, removeHandler is available anyway..

[Aiosa]

Also, I noticed that

• addOnceHandler does not really work with removeHandler - I updated the docs, but is this desirable?
• exceptions are not caught in execution, possibly exiting the whole event

[iangilman]

Wow, there's a lot going on here!

• I'm glad to have the detection for async functions working.
• Adding priority to the EventSource is a lovely addition, and the implementation looks nice and clean.
• I think it's fine that addOnceHandler doesn't work with removeHandler... It's just a convenience function. Good to have it documented though.
• I'm concerned about adding support for promises in the EventSource. What's the scenario you're trying to support?
• Please tell me more about "ability to exit raised event for all unfinished callbacks"... What's the scenario there?

I don't think we should be adding complexity without strong scenarios to back it up. Also, the scenarios can help us decide what tradeoffs to make.

Thank you!

[Aiosa]

These two features were added after I was trying to re-use this event system in a more complex architecture. Although these do not necessarily bring immediate merit to OSD, I needed these for a fully-fledged event system.

Exiting event from a callback
Originally, I was having a UI Dialog system that handled certain events arising with attached system components (errors, warnings) and wanted to have the ability to both order (priority) and exit so that e.g. default message can be shown if no one handles a certain error type and vice versa. These do not have to be messages but different kind of actions - sort of 'exception-like' handling across a system without actually using exceptions.

Also, one could elegantly remove default events without actually removing them by creating a higher-priority callback that exits the event. This could (temporarily) bypass OSD features via events. This would be even more powerful if events in OSD had defined priority numbers.

Promises in EventSource
Promises were always possible in EventSource - just set set callback

events.addHandler('event', () => new Promise( resolve => { ... async code ... ;  resolve(); } ));

Since OSD now recognizes async function as a function, it is even easier to do so. The only thing that events can do with such a function is to fire all events without synchronization. This feature makes priority feature useless so for consistency, one would like to have an option that keeps the priority of functions in events. Another use scenario is when a function tries to process data while being forced to use asynchronous calls (fetch, IO of third-party libs) and another party (lower-priority callback or event caller) wants to continue after such callback finished. In my case, this was with IO across a system - an event was called that data is exported and modules used the event to provide their data. Naturally, some required async execution, but the event source had to wait for all to finish before assembling the export.

Recognizing async function as a function

I'm glad to have the detection for async functions working.

Although it was allways possible to create a function that returns its logic in a promise and pass it as a callback, still, you had to go and implement it. async is just a fancy keyword one might slap in front of a function and be surprised that everything broke down.

Wouldn't it be better to create $.isAsyncFunction and allow these on explicit places (such as EventSource)?

[iangilman]

Thank you for the explanations!

Exiting event in callback

My concern here is whether there are any internal systems in OSD that depend on those events getting through. We've always had the assumption that they would, so if that were to change, we'd want to really make sure it's not going to cause problems.

From an API standpoint, I feel like the existing preventDefault flags are maybe just a little easier than making a high priority event specifically for the purpose of exiting the event stream. Of course, the nice thing about the proposed exit feature is that it covers all of the places where we may have missed putting in preventDefault.

Again, I haven't examined all the code, but I could also imagine situations where preventDefault doesn't just immediately stop, but does some necessary processing, in which case, just exiting the event might be too brutal.

So... It seems to me more cost/benefit research is needed on this.

Promises in EventSource

I'm also concerned that this might disrupt internal OSD things. Again, we'd just have to look I guess.

I'm also not sure there's a scenario where it's necessary to do this sort of event chaining within OSD... There's nothing stopping you from event chaining inside your own handler. I guess the scenario is when you're going to need to do something lengthy, like hit a server, before telling OSD it's okay to proceed. Do you have a specific scenario like that in mind?

One of OSD's top priorities is performance and responsiveness. The bedrock of that is for every action, we do what we can with the info we have at the moment. I don't like the idea of potentially adding hiccups between a user action and OSD responding to it. A better design pattern is to handle this action with the info you have and fire off whatever async things you need in order to have the right info you may want for the next action.

Anyway, I'd love to hear about the specific scenario!

Recognizing async function as a function

Wouldn't it be better to create $.isAsyncFunction and allow these on explicit places (such as EventSource)?

So reverting $.isFunction so it gives an erroneous result? Or you mean keeping that fix, but also adding $.isAsyncFunction`? I suppose having both is fine, especially if we need it. I guess it depends on how the other discussions go.

In general, I'm not in favor of adding things into OSD that can be handled outside of OSD; otherwise, the library just grows in complexity. If the scenarios you're interested in can be done outside of OSD using the existing API, I think we should stick with that.

[Aiosa]

Exiting event in callback

Ok, let's remove it for now. I guess using a flag is also an option.

Promises in EventSource

I'm also concerned that this might disrupt internal OSD things. Again, we'd just have to look I guess.

I was on the other hand concerned about not having this would disrupt things. My primary motivation for adding this was that one would expect its presence in the API. We can remove it. We can move it to a new method raiseAwaitEvent() or similar. We can remove it, make it into a plugin or ... I still think having it there makes sense, but I don't have your overview of the lib. Please decide.

Recognizing async function as a function
Wouldn't it be better to create $.isAsyncFunction and allow these on explicit places (such as EventSource)?

So reverting $.isFunction so it gives an erroneous result? Or you mean keeping that fix, but also adding $.isAsyncFunction`? I suppose having both is fine, especially if we need it. I guess it depends on how the other discussions go.

Keeping disjunct isFunction and isAsyncFunction so that one explicitly enables the use of async function as an argument. Events would check callback by calling both, other OSD parts would not which would keep the old behavior that is sure to be safer. My concern is that fixing this function breaks stuff in OSD since isFunction historically does not work with async functions, the code reflects it (using results of callbacks without await or similar..)

[iangilman]

Exiting event in callback

Ok, let's remove it for now. I guess using a flag is also an option.

Cool. We can always revisit later if need be.

Promises in EventSource

I was on the other hand concerned about not having this would disrupt things. My primary motivation for adding this was that one would expect its presence in the API. We can remove it. We can move it to a new method raiseAwaitEvent() or similar. We can remove it, make it into a plugin or ... I still think having it there makes sense, but I don't have your overview of the lib. Please decide.

I think we should keep the API focused on real-time actions. If async stuff needs to be introduced, it should be on a case-by-case basis, or, ideally, something the developer does outside of the internal OSD workings. That said, if you have an example of a scenario where not supporting promises in events would cause disruptions, I'd love to hear about it. Otherwise, yes, let's set that aside.

Recognizing async function as a function

Keeping disjunct isFunction and isAsyncFunction so that one explicitly enables the use of async function as an argument. Events would check callback by calling both, other OSD parts would not which would keep the old behavior that is sure to be safer. My concern is that fixing this function breaks stuff in OSD since isFunction historically does not work with async functions, the code reflects it (using results of callbacks without await or similar..)

The async keyword is just syntactic sugar for "this function returns a promise", right? I think having isFunction return true for such functions makes sense; they are, after all, still functions. In cases where we're not expecting a return value, it doesn't matter what it returns. In cases where we are expecting a return value, a promise is just an incorrect return value, no different than if the function returned a string when it was supposed to return an object. The correct way to test for an incorrect return value would be after calling the function; if we have places where that's a problem, we should have guards there.

[Aiosa]

The async keyword is just syntactic sugar for "this function returns a promise", right?

It is almost the same. There are some differences but the main idea is that both return promise because of the (backward) compatibility of this approach. Should be OK now?

[iangilman]

Looks great! Thank you for working with me on what to include here. Lots of factors go into what's best for OSD. ❤️