Bug in Hype Events that always only returns last value (like HypeLayoutRequest)

1

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.

2

Haven't tested it, but this might be a solution… what do you think?

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 === false) {
                return false;
            }
            if (callbackResult !== undefined) {
                result = callbackResult;
            }
        }
    }
    return result;
};
3

I think it comes down to which is conceptually cleaner:

  1. ordering matters for false and last result
  2. 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:

  1. 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;
};
2 Likes
4

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.

1 Like
5

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.

6

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

1 Like
7

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.

8

Yes, I like generic solutions.

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…

1 Like
9

Yes - I think that's the route we'll go. Thanks!

1 Like