feat: add lwc:on directive

[gaurav-rk9]

Details

Implement lwc:on as specified in RFC: lwc:on Directive

differences from RFC :

• When lwc:on={eventHandlers} is used, the object passed, i.e. eventHandlers will be freezed, and any attempt to modify it shall throw a runtime error. However eventHandlers can be re-assigned to a different object, and this shall update the actual event listeners correspondingly, i.e. removal of previous event listeners and addition of new ones.
• disallows usage of onXXX and lwc:on together on HTML templates.

Does this pull request introduce a breaking change?

• 😮‍💨 No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

• 🔬 Yes, it does include an observable change.

Allows the use of new lwc:on directive

GUS work item

W-17411303, W-17632714

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

[wjhsf]

Can we add a test case for a computed key? Something like ['computed'], just to see what happens.

[gaurav-rk9]

I don't understand what would be the benefit of this test.
My understaning is when computed property syntax in used to create an object, once the object is created, the object doesn't have any memory of computing expression. It is indistinguishable from the object which is created directly.

[wjhsf]

Actually, to catch the potential issue I'm concerned about, it should be [computed] (identifier, not string literal). The LWC compiler walks the AST of the code to apply transformations, and we want to validate that the transformations are applied correctly. I want to make sure we avoid a scenario like #4988, where the input {[computed]: x} is transformed to {computed: x}, rather than {[computed]: x}.

[gaurav-rk9]

No transformation is applied to the object itself by the template compiler. It only works with the identifier representing the object, not the object directly. The meaning of that identifier is determined elsewhere, and this is a common abstraction used for all attributes in the template that accept variables as arguments — it’s not specific to lwc:on.
So, I think to catch any issues arising from "the meaning of that identifier" being incorrect, we should have separate tests that specifically verifies that.

[divmain]

I think what Will wants here is a sanity check in case the implementation changes in the future. You're correct that the object assigned to eventHandlers will not be touched by the template compiler. But it isn't impossible to imagine changes to the JS compiler in the future to resolve some bug with lwc:on or an adjacent feature.

Even the most innocuous code can be problematic when enough component authors are depending on niche observable behavior. So adding a test case here (while it may seem unnecessary) is a prudent thing to do.

[wjhsf]

I feel like, historically, we've made things like this just console warnings that only throw in dev mode. I also feel like, currently, we've decided that's an annoying pattern that nobody pays attention to, so maybe we should just go with this. @divmain, thoughts?

[divmain]

Yes, we have typically used logError for this. But in this case, I think I prefer throwing an error - as you say, nobody pays attention to the warnings. And if we don't throw, somebody will eventually do the wrong thing and expect us to make it work.

[gaurav-rk9]

I didn't notice earlier that logError is used elsewhere, In that case I would want to change this to logError since we would want that (reactivity with same object, modified) to be available if need has been demonstrated.
Also, that case (same object passed to lwc:on) works just as fine, if we don't throw, so there is no cost of 'making it work'.
@divmain , is it fine if I make the change?

[divmain]

Yes feel free.

[gaurav-rk9]

done

[divmain]

Just a few minor changes requested, but otherwise looks really good! Thanks for your work on this! Let us know when you've addressed all comments, and we'll get this merged!

[divmain]

Yes, we have typically used logError for this. But in this case, I think I prefer throwing an error - as you say, nobody pays attention to the warnings. And if we don't throw, somebody will eventually do the wrong thing and expect us to make it work.

[gaurav-rk9]

Do we need to add any flags to toggle the availability of this feature, or is it not required?

[divmain]

Do we need to add any flags to toggle the availability of this feature, or is it not required?

Realistically, yes @gaurav-rk9. Our policy is to add flags (and internal gates) for substantial changes. This qualifies, even if it is a net-new functionality. Probably the best place to do this is in the template parser, so that lwc:on nodes are never recognized as something special. Then, the compiled template will not include the calls to the new runtime logic you've added.

[divmain]

One last thing: there probably needs to be a small change in the SSR compiler to handle lwc-on template AST nodes by simply not emitting anything. That can be done in a followup PR, however.

[gaurav-rk9]

One last thing: there probably needs to be a small change in the SSR compiler to handle lwc-on template AST nodes by simply not emitting anything. That can be done in a followup PR, however.

It may take some time to understand SSR compiler, so I will do it in a followup PR

[wjhsf]

@divmain should we use API versioning to introduce this change? It's a good candidate to incentivize people to upgrade.

[wjhsf]

People try a lot of weird things, can you add some tests for some expected edge cases? Please add a test with lwc:on={eventHandlers} for each of these possible eventHandlers:

• null
• undefined
• string
• function
• defined as a setter on the class (without a getter)
• object with a getter that throws
• object with non-function values
• new (class extends LightningElement {}) (and validate that a render event doesn't call the component's render method)

[gaurav-rk9]

People try a lot of weird things, can you add some tests for some expected edge cases? Please add a test with lwc:on={eventHandlers} for each of these possible eventHandlers:

• null
• undefined
• string
• function
• defined as a setter on the class (without a getter)
• object with a getter that throws
• object with non-function values
• new (class extends LightningElement {}) (and validate that a render event doesn't call the component's render method)

Thanks, some of these are fundamental tests for this feature, which I missed. The undefined case helped me find a mistake in my implementation.
Added these tests !!

[divmain]

Looks like this PR is just about ready. However, we are going to hold off merging until we cut a 256 release branch. This will be included with SF 258 release.