{{on}} modifier should not add event listeners if the passed function is undefined

[NullVoxPopuli]

the On modifier's addEventListener helper function: https://github.com/emberjs/ember.js/blob/a493c6a840ae6e56a612237c2afa495ed2cdfd50/packages/%40ember/-internals/glimmer/lib/modifiers/on.ts#L211

always adds an event listener (here: https://github.com/emberjs/ember.js/blob/a493c6a840ae6e56a612237c2afa495ed2cdfd50/packages/%40ember/-internals/glimmer/lib/modifiers/on.ts#L392 )

I think it'd help out a lot of folks to make this change to update:

https://github.com/emberjs/ember.js/blob/a493c6a840ae6e56a612237c2afa495ed2cdfd50/packages/%40ember/-internals/glimmer/lib/modifiers/on.ts#L374

    // use prior state values for removal
    removeEventListener(element, eventName, callback, options);

    // read updated values from the state object
-   addEventListener(state.element, state.eventName, state.callback, state.options);
+   if (state.callback) {
+     addEventListener(state.element, state.eventName, state.callback, state.options);
+    }

[buschtoens]

I already suggested that in the past and the recommendation was to conditionally apply the modifier once the RFC that allows passing around modifiers lands.

I don't have the links at hand right now, because I'm on mobile. #660 (comment)

[NullVoxPopuli]

I think that could def help, but I believe it to be mostly orthogonal to this issue?

[buschtoens]

I agree, but IIRC the line of reasoning was that not calling the modifier in the first place is more efficient than calling it and have it effectively no-op.

Wit the other RFC in place, you could basically use an {{if}} expression in place of the modifier:

<button {{if @handler (modifier "on" "click" @handler)}}>

[jelhan]

Wondering if this should be considered a bug? Attaching an event listener with a listener, which is neither a function nor implements EventListener interface is a bug. The {{on}} modifier should prevent this.

But it's another question if it should support undefinded or any other value, which isn't a valid listener. Maybe it should just fail early with a helpful error message? To be honest I'm tending towards that solution.

Silently not applying an event listener is error prune and hard to debug. I can imagine myself spending hours until I notice that there is a typo. 🙈

But I agree that core misses a simple pattern to conditionally apply an event listener. An alternative solution might be returning a noop function if {{fn}} helper is called without any argument. Currently an error is thrown in that case (with an error message which isn't very helpful by the way).

I'm not sure about the performance impact of applying unneeded event listeners with a noop function. But I expect it to be acceptable for most cases. Also it would be more flexible as that pattern could be used at all places which expects a function - and not only for {{on}} modifier.

[sukima]

This isn't a good option but I do this:

{{on "click" (if @foo @foo this.noop)}}

I don't know seems silly to bike shed when there is no solution landed anyway. I really dislike dropping ideas in favor of future work which is not available yet. It is like saying "that thing you needed is not important enough for a solution today." 🤷

[lukemelia]

Another workaround, which we use, is the optional helper from ember-composable-helpers:

{{on "click" (optional @foo)}}

[buschtoens]

Here's the RFC I was referring to: #432 Contextual Helpers and Modifiers (a.k.a. "first-class helpers/modifiers")
And here's the tracking issue: emberjs/rfc-tracking#6

[buschtoens]

We also use (optional), like @lukemelia suggested (#660 (comment)) and I think it is superior to making the callback parameter itself optional, like suggested by @NullVoxPopuli in the OP #660 (comment).

As @jelhan has already pointed out (#660 (comment)), this would masquerade typos.

An alternative solution might be returning a noop function if {{fn}} helper is called without any argument.

This would pose the exact same hazard, just in a different place.

IMO using (optional) is the perfect solution, while RFC #432 is still pending implementation. It's a bit unfortunate that it's not part of Ember Core, but I think that's okay.

[NullVoxPopuli]

Atm, there are two related goals

• clean code for falsey or undefined values
• not adding on event listener when the value is undefined

Personally, I care more about the second.

[jelhan]

An alternative solution might be returning a noop function if {{fn}} helper is called without any argument.

This would pose the exact same hazard, just in a different place.

Are you sure? I suppose that {{fn}} returns a noop function if called without any argument. Calling it with an argument which is not a function (e.g. {{fn undefined}}), should still be invalid and throw.

[NullVoxPopuli]

fn already solely means 'partial application' though?

[jelhan]

fn already solely means 'partial application' though?

You are right that the meaning of {{fn}} helper would slightly change. I propose that it could be summarized like this:

The {{fn}} helper returns a new function.

You may pass a callable as first positional argument. The returned function calls that callable with all additional positional arguments given.

If no positional argument is given a noop function is retuned.

The current API docs are missing a technical explanation like that. They only talk about the use cases (why) but not about the technical implementation (what).

[buschtoens]

@jelhan The {{fn}} helper expects the first argument to always be a function:

https://github.com/emberjs/ember.js/blob/b049c37e66587718cef38cde63b59b0789505d43/packages/%40ember/-internals/glimmer/lib/helpers/fn.ts#L82-L92
https://github.com/emberjs/ember.js/blob/b049c37e66587718cef38cde63b59b0789505d43/packages/%40ember/-internals/glimmer/lib/helpers/fn.ts#L97-L102

Allowing {{fn undefined}} to create a no-op function, like {{optional}} does, IMO is dangerous, as it opens the door for typo bugs, much in the same way that it would for {{on}}.

Going back to the two design goals by @NullVoxPopuli:

• clean code for falsey or undefined values
• not adding on event listener when the value is undefined

The first one IMO is sufficiently covered by {{optional}}. The second one can be solved either, when RFC #432 is implemented, as explained above, or by adding some param like optional=true to the {{on}} modifier:

<button {{on "click" @onClick optional=true}}>

I agree very much, that both goals are super desirable, but I don't think increasing the API surface area or loosening the safety net is a good idea.