-
Notifications
You must be signed in to change notification settings - Fork 78
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
Adds support for event proxying and event options #856
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit f2f2d67:
|
8d2bc7d
to
d570feb
Compare
Codecov Report
@@ Coverage Diff @@
## master #856 +/- ##
==========================================
- Coverage 94.83% 94.71% -0.12%
==========================================
Files 127 127
Lines 8049 8085 +36
Branches 1870 1884 +14
==========================================
+ Hits 7633 7658 +25
- Misses 416 427 +11
Continue to review full report at Codecov.
|
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.
Just a couple of type suggestions, otherwise 👍
src/core/vdom.ts
Outdated
if (eventCallback) { | ||
domNode.removeEventListener(eventName, eventCallback); | ||
const proxyEvents = _eventMap.get(domNode) || {}; | ||
let proxyEvent: { proxy: EventListener; callback: Function; options: any } | undefined = |
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.
Could this be EventMapValue | undefined
?
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.
yeah it can actually just be inferred, i just forgot to remove it.
src/core/vdom.ts
Outdated
@@ -1236,6 +1236,8 @@ function wrapFunctionProperties(id: string, properties: any) { | |||
return props; | |||
} | |||
|
|||
type EventMapValue = { proxy: EventListener; callback: Function; options: any } | undefined; |
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.
Could we type options? passive: boolean
or similar?
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 wanted to actually use EventListenerOptions
but I think the type didn't have passive. I'll type it though.
Type: feature
The following has been addressed in the PR:
prettier
as per the readme code style guidelinesDescription:
This adds support for 2 features:
Event Proxying
Before this change, events were removed and re-attached if the callback changed across renders. Unfortunately this is a very common occurrence due to inline functions encouraged. With this change, a wrapping proxy is used which simply points to the callback, which means we do not have to re-attach the event.
Event Options
Added a new special typed property called
oneventoptions
which currently supportspassive
; an array of supported eventlisteners. This allows you to set whether an event is passive or not.(https://developers.google.com/web/updates/2016/06/passive-event-listeners).Squatting on a property on a
VNodeProperties
is not ideal, but it is at least prefixed with the existingon
squat. And this method is the least intrusive to the existing behaviour.Example usage:
Event options are not supported on
addEventListener
at all in older browsers (IE) so a sniff has been added tohas
calleddom-passive-event
.Resolves #810