The runtime uses the following notifyEvent function:
var notifyEvent = function(event, element) {
var eventListeners = window['HYPE_eventListeners'];
if (eventListeners == null) {
return;
}
var result;
for (var i = 0; i < eventListeners.length; i++) {
if (eventListeners[i]['type'] == event['type'] && eventListeners[i]['callback'] != null) {
result = eventListeners[i]['callback'](_hype['API'], element, event);
if (result === false) {
return false;
}
}
}
return result;
};
But if an event listener that is registered twice and has a return value runs, it takes the last result even if that is null. Shouldn't it rather use a test to see if the result is not null/undefined before setting it? Thoughts/Solutions? I enqueued my HypeLayoutRequest in a certain order to have the one that actually changes the layout last, but this is a very rigid and not very flexible solution.
I think it comes down to which is conceptually cleaner:
ordering matters for false and last result
ordering matters for false, but doesn't matter for the last result
It is actually unclear at this point in history if I even wanted false to be a "stop notifying other handlers" flag, the deeper bug may be that this should not immediately return. So the cleanest could be:
Last returned result matters
I doubt there are many people adding multiple handlers for the same event (intentionally with different code, anyhow). Are there places where this solution might break you?
var notifyEvent = function(event, element) {
var eventListeners = window['HYPE_eventListeners'];
if (eventListeners == null) {
return;
}
var result;
for (var i = 0; i < eventListeners.length; i++) {
if (eventListeners[i]['type'] == event['type'] && eventListeners[i]['callback'] != null) {
var callbackResult = eventListeners[i]['callback'](_hype['API'], element, event);
if (callbackResult !== undefined) {
result = callbackResult;
}
}
}
return result;
};
In my case, I use the events in my extensions, and they can be added in arbitrary order (either load order) or order in the file that adds multiple of them. I think your suggestion is a suitable solution because:
It doesn't overwrite a previous result with undefined
It doesn't exit prematurely if a false is encountered
Last one wins, but at least one can listen to events like HypeLayoutRequest without breaking logic if one doesn't return anything.
I don't see a downside to this fix at the present moment.
What do you think? I'll patch it on Monday and try it out in a actual project.
Yeah, that's what I was figuring your use case would be, and thus the false-to-stop-processing is probably not good behavior if multiple "clients" don't know about each other.
I think in an ideal world if you returned a layout from this, then the next event might also get this new layout in the layoutName field. However as it stands now this would be a much more invasive change.
If it looks good to you then i'll just make this change. It is the type of thing though where if it winds up breaking folks I'd just revert it back ASAP.
You mean something like this, adding to the signature?
var notifyEvent = function(event, element) {
var eventListeners = window['HYPE_eventListeners'];
if (eventListeners == null) {
return;
}
var result;
for (var i = 0; i < eventListeners.length; i++) {
if (eventListeners[i]['type'] == event['type'] && eventListeners[i]['callback'] != null) {
// Pass the previous result as an new optional argument to the callback function
var callbackResult = eventListeners[i]['callback'](_hype['API'], element, event, result);
if (callbackResult !== undefined) {
result = callbackResult;
}
}
}
return result;
};
Or splicing it into event… but I think this is cleaner.
That way, one could act on previous results if needed (like passing it along or refraining from changing it).
I was thinking that the event record would have a hypeProposedLayoutName (what was originally proposed) and a layoutName which would be what might have been set by a previous caller.
Passing the current result is an interesting idea. There's less context, but it is more generic.
Regardless, I'm not sure previous results will really matter much to functions that are chained in - I think I will just file this as an idea and wait for use cases.
So, the fix for the force false abort and setting response to unrefined initially discussed is still in the run, but the response value idea is on the shelf for now. Did I sum that up, correctly. Fine by me…