Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
lwc:on
Directive #92base: master
Are you sure you want to change the base?
RFC:
lwc:on
Directive #92Changes from 2 commits
0a7dd5b
a629fa0
45f3aa0
fa131b6
86c1325
0396d6a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if one of these examples used
this
, to demonstrate that the listeners are bound to the component instance.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense to me. If something is dynamically applied, it "feels" like it should take precedence. So
onfoo
is essentially the "default" listener.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thoughts on this were
lwc:spread
works.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense to me. Under the hood, it will be implemented similar to how
lwc:spread
is implemented, i.e.lwc:spread
is:and
lwc:on
would be:(I'm skipping the binding and caching, but you get the idea.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My initial thought was that
onfoo={handleFoo}
andlwc: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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently LWC throws a compile-time error if you try to use
onfooBar
oronfoo-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 inlwc:on
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thetype
. we just let the browser'saddEventListener
throw.we should do the same here right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the object iterators (
Object.keys
,for...in
, etc.) ignore symbols, so any naive implementation oflwc: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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, except when
lwc:component
is used for dynamic component creation since the events are bound after theconnectedCallback
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Static Analysis since it's a dynamic component we would run into similar issues as we do with
lwc:component
in that we cannot easily search for what components these create in the client code.