RFC: lwc:on Directive

[gaurav-rk9]

This proposal adds a declarative mechanism to add a collection of event listeners to elements in an LWC template using a new directive lwc:on.

[salesforce-cla]

Thanks for the contribution! Unfortunately we can't verify the commit author(s): Gaurav Kochar <g***@s***.com>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, refresh the status of this Pull Request.

[Templarian]

Yes, except when lwc:component is used for dynamic component creation since the events are bound after the connectedCallback.

[Templarian]

For Static Analysis since it's a dynamic component we would run into similar issues as we do withlwc:component in that we cannot easily search for what components these create in the client code.

[nolanlawson]

This proposal actually looks great to me. I think using lwc:on is much cleaner than the previous lwc:spread-based proposal.

One edge case not covered: are we planning to just use a simple Object.keys() on the supplied object? Or Object.getOwnPropertyNames? This matters if someone passes in e.g. a subclass that inherits properties from a superclass.

Another thing missing from this document is a review of what other frameworks are doing – how is Svelte, Vue, Lit, Solid, React, etc. handling this case?

[nolanlawson]

This makes sense to me. If something is dynamically applied, it "feels" like it should take precedence. So onfoo is essentially the "default" listener.

[ekashida]

Does it make sense that only one or the other would be bound to the element, when you're generally able to bind the same event more than once?

<template>
    <x-counter lwc:ref="counter" onfoo={handleFoo}></x-counter>
</template>

import { LightningElement } from 'lwc';
export default class extends LightningElement {
  handleFoo() {
    console.log('foo static')
  }
  renderedCallback() {
    this.refs.counter.addEventListener('foo', () => {
      console.log('foo dynamic');
    });
    this.refs.counter.dispatchEvent(new CustomEvent('foo'));
  }
}

> foo static
> foo dynamic

[gaurav-rk9]

My thoughts on this were

• Since we don't provide consumers with a way to explicitly remove these listeners, To execute multiple functions when an event is recieved, One can create a wrapper handler over these functions with added guarantee of order.
• Fallback event listener seems to be of more value than an additional unconditional event listener
• This would be consistent with how lwc:spread works.
• This directive should not be a way for having multiple event listeners. If at some point we want to support multiple declarative event listeners, we should have a clean way to do that and it should not be a side effect of something else.

[nolanlawson]

This would be consistent with how lwc:spread works.

This makes sense to me. Under the hood, it will be implemented similar to how lwc:spread is implemented, i.e. lwc:spread is:

props: {
  foo: $cmp.foo,
  ...$cmp.someObject
}

and lwc:on would be:

on: {
  foo: $cmp.foo,
  ...$cmp.someObject
}

(I'm skipping the binding and caching, but you get the idea.)

[wjhsf]

My initial thought was that onfoo={handleFoo} and lwc:on={ /* foo: handleFoo */ } should allow both, because having multiple event handlers is conceptually fine. But it's also reasonable to say "there's just one way of doing things, don't mix and match", to avoid user confusion and combinatorial complexity.

[nolanlawson]

It would be great if one of these examples used this, to demonstrate that the listeners are bound to the component instance.

[nolanlawson]

Currently LWC throws a compile-time error if you try to use onfooBar or onfoo-bar or other disallowed naming patterns. Are you proposing that we throw an error at runtime? Or warn? Or something else?

FWIW this was actually a point of controversy in the Custom Elements Everywhere benchmark, which LWC does not fully pass because we don't support these event names.

I think there's a case to be made that we should not be so strict here, and should just accept whatever event string name the user passes in. (Except maybe nonsensical values like the empty string '', non-strings, etc.) There are cases of web components "in the wild" that use odd naming conventions for their event names, and it would be nice to support that in lwc:on.

[gaurav-rk9]

My understanding of why this is disallowed is based on Summary of rfc: declarative binding for non-standard event names.
we don't have any of those constraints here , so in long term I don't think we must have these restrictions.
However, I think when event names are known while authoring the component, the approach finalized in rfc: declarative binding for non-standard event names should be the recommended approach due to better static analyzibility.
So maybe we should have these restrictions until rfc: declarative binding for non-standard event names is implemented.

[ekashida]

I agree that we should just support whatever the spec supports (i.e., DOMStrings), which is any valid string. I think the only thing we'd need to look out for are Symbols, as the spec allows either a string or a symbol as the key in an object.

Could you mention that we'll filter out Symbols?

[gaurav-rk9]

currently , when consumers use addEventListener, (https://github.com/salesforce/lwc/blob/8d3bc0848e77fb1a2f74019d570dbfb684010c0c/packages/%40lwc/engine-core/src/framework/restrictions.ts#L182, https://github.com/salesforce/lwc/blob/8d3bc0848e77fb1a2f74019d570dbfb684010c0c/packages/%40lwc/engine-core/src/framework/base-lightning-element.ts#L336) , we don't do any checks on the type. we just let the browser's addEventListener throw.
we should do the same here right?

[wjhsf]

All of the object iterators (Object.keys, for...in, etc.) ignore symbols, so any naive implementation of lwc:on will likely implicitly ignore symbols. If we want to check for symbols, we'd have to do so explicitly. That seems unnecessary, though, because they're not valid event names and probably nobody will ever try to use them.

[gaurav-rk9]

I agree that the implementation would be simpler, my thought was what would consumer expectation be, but now that I think about it , it does seem reasonable to say only own enumerable string-keyed properties will be considered.

[nolanlawson]

I doubt it would be significant, FWIW.

In my mind, the best argument against lwc:spread is 1) it's a potentially breaking change, and 2) its a potential point of confusion. lwc:on is much cleaner in my opinion because the intention is clear.

[Templarian]

@nolanlawson I'm fine with either approach, but was under the impression lwc:spread would complicate the existing compiled code. Ex: lwc:spread={simpleProps},

    props: {
      "name": "template",
      ...$cmp.simpleProps // <- This would need a runtime filter out .startsWith('on')
    },

[nolanlawson]

Sure, it would complicate our code, but that's not the main argument against it IMO. Yes you're right that we would need a filter there.

[gaurav-rk9]

Updated this section. I am not sure what qualifies as significant, I have removed it for now.

[salesforce-cla]

Thanks for the contribution! Before we can merge this, we need @gaurav-rk9 to sign the Salesforce Inc. Contributor License Agreement.

[wjhsf]

consumers understand their differences
a combined directive might lead consumers to treat them as a unified concept, causing confusion

counterpoint: consumers are already confused 😛

[gaurav-rk9]

Well, we wouldn't want to confuse them more. 😃

[wjhsf]

My initial thought was that onfoo={handleFoo} and lwc:on={ /* foo: handleFoo */ } should allow both, because having multiple event handlers is conceptually fine. But it's also reasonable to say "there's just one way of doing things, don't mix and match", to avoid user confusion and combinatorial complexity.

[wjhsf]

So this is essentially just sugar for doing addEventListener once and moving on? We wouldn't care/explode if a user did something weird like

{
  foo: function handleFoo() {
    this.removeEventListener('foo', handleFoo)
  }
}

[gaurav-rk9]

Yes, this would do addEventListener once immediately after construction and move on. We don't care what the function does, we take the function input, wrap it into a new function that binds it to the owner component and and then add it as event handler to the element.

{
  foo: function handleFoo() {
    this.removeEventListener('foo', handleFoo)
  }
}

Here if the expectation is to remove the event listener added for event foo, this won't work as expected, because the event handler added won't be handleFoo, it will be a different function, whose reference the consumer doesn't have.

[Templarian]

Removing the event handler dynamically placed on the host doesn't seem like it is a required feature for these once attached.