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

Adds support for event proxying and event options #856

Merged
merged 11 commits into from
Oct 26, 2020

Conversation

matt-gadd
Copy link
Contributor

@matt-gadd matt-gadd commented Oct 23, 2020

Type: feature

The following has been addressed in the PR:

Description:
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 supports passive; 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 existing on squat. And this method is the least intrusive to the existing behaviour.

Example usage:

<div 
  onscroll={ () => { /* whatever your event callback is */ } } 
  oneventoptions={ { passive: ['onscroll'] /* sets the above event to be passive */ } }
>Scrollable Content</div>

Event options are not supported on addEventListener at all in older browsers (IE) so a sniff has been added to has called dom-passive-event.

  • Upgraded jsdom to support passive events
  • Upgraded webcomponents polyfill for tests due to some strange behaviour/interaction with events in IE

Resolves #810

@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 23, 2020

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:

Sandbox Source
dojo/dojo-codesandbox-template Configuration

@codecov
Copy link

codecov bot commented Oct 26, 2020

Codecov Report

Merging #856 into master will decrease coverage by 0.11%.
The diff coverage is 98.18%.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
src/core/vdom.ts 97.95% <97.67%> (+0.03%) ⬆️
src/core/has.ts 96.39% <100.00%> (+0.23%) ⬆️
src/shim/tslib.ts 54.16% <0.00%> (-45.84%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fbef711...f2f2d67. Read the comment docs.

Copy link
Member

@agubler agubler left a 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 =
Copy link
Member

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?

Copy link
Contributor Author

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;
Copy link
Member

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?

Copy link
Contributor Author

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.

@matt-gadd matt-gadd merged commit 98ec48f into dojo:master Oct 26, 2020
@matt-gadd matt-gadd mentioned this pull request Nov 18, 2020
3 tasks
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.

Passive event listeners
2 participants