Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rfc: declarative binding for non-standard event names #88

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ekashida
Copy link
Member

@ekashida ekashida commented Feb 28, 2024

Copy link
Contributor

@nolanlawson nolanlawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great exploratory RFC, but I think we should pin it down to a particular proposal and evaluate the pros/cons of that. There are a few different options being weighed here, but I think the onFoo-BarBaz variant is the most viable, to me anyway.

<third-party lwc:external lwc:on:Foo-Bar-BAZ={handleConnected}></third-party>

<!-- Event as part of attribute name (special treatment for lwc:external) -->
<third-party lwc:external onFoo-Bar-BAZ={handleConnected}></third-party>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, this option is the least disruptive. Yes, it is playing fast-and-loose with HTML semantics, but it is the most similar to what we have today, avoiding the need for a special new lwc:on directive or other mechanism.

Scoping it to lwc:external also brings that advantage that we can pass 100% on Custom Elements Everywhere without changing how LWC components have worked up until today. (We can re-evaluate that later if needed, but I really think it's not a big deal – the important thing is just to support the wide world of third-party custom elements and their wacky naming conventions for events.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it's the least disruptive, but I also feel like it might be a source of confusion/surprise. The nice thing about introducing a new directive is that it's easy to understand...if you want to do this thing that you were never able to before, you have to use this new directive.

PascalCase: (event) => console.log("'PascalCase' handled"),
};
}
```
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This to me is unnecessary since we already have lwc:spread, which works with onclick/onblur/etc. We may have to consider how lwc:spread would interact, though, with lwc:external if we decided to support the non-standard event names there.

used to listen for standard events like `click` or `focus` by assigning event listeners to the
`onclick` and `onfocus` properties. However, adding support for non-standard event names would add
complexity to `lwc:spread`. Introducing a new directive also makes it easier to restrict the usage
of `lwc:on` to `lwc:external` components.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It certainly adds complexity if we want to maintain the difference between lwc:external vs LWC components. We'd have to add validation to the client side (runtime) as well as at compile-time. Although we could just not support lwc:spread with non-standard event names entirely – we'd still score 100% on Custom Elements Everywhere, so the value is debatable.


## lwc:on=`${eventName}:${listenerName}`

Encoding both the event name and event handler name as a string using some delimiter would allow us
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a big fan of lwc:on, especially since Svelte v5 is actually moving away from their existing on:click syntax towards a syntax more like ours, since it plays nicer with their equivalent of lwc:spread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants