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

WIP(compiler): fix #19878, can config zone/once/passive/capture in template/HostListener decorator #21681

Closed
wants to merge 1 commit into from

Conversation

JiaLiPassion
Copy link
Contributor

@JiaLiPassion JiaLiPassion commented Jan 20, 2018

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #19878

can't pass passive, capture, once parameters to addEventListener.

not easy to config use ngzone or nozone with HostListener

user need to explicitly call runOutsideOfAngular to make sure eventhandler run outsideofngZone

@HostListener('window:resize', ['$event.target'])
  onResize(target: any) {
    this.ngZone.runOutsideOfAngular(() => {
       console.log('resize triggered');
    });
  }

or call ngZone.run explicitly make sure eventhandler run into ngZone.

@HostListener('window:resize', ['$event.target'])
  onResize(target: any) {
    this.ngZone.run(() => {
       console.log('resize triggered');
    });
  }

not easy to run event handler outsideOfAngular in template

<div (mouseover)="mouseover();"></div>
mouseover() {
  this.ngZone.runOfAngular(() => {
    ...
  });
}

What is the new behavior?

Can config which zone will event handler will run into in @HostListener decorator.

can config passive, capture, once parameters to addEventListener.

  • in template, can config those parameter like below.
<div (mouseover.capture.once.passive)="mouseover();"></div>

can config ngzone which will guarantee eventhandler run in ngZone even the handler was

added not in ngZone.

 @HostListener('window:resize.ngzone', ['$event.target'])
  onResize(target: any) {
    console.log('resize triggered');
  }
  • can config nozone which will guarantee eventhandler run outside of ngZone even the handler was added in ngZone. and if config outsideOfAngular, DOMEventManager will bypass zone.js and directly use native addEventListener to improve performance.

And it can also work with #20672 to get better performance.

 @HostListener('window:resize.nozone', ['$event.target'])
  onResize(target: any) {
    console.log('resize triggered');
  }

Does this PR introduce a breaking change?

[] Yes
[x] No

Other information

@devoto13
Copy link
Contributor

@JiaLiPassion I use custom EventManager implementation in my project to tackle same problem. The idea is pretty similar to what is done here. But weakly typed. I would be very interested to have a built-in way to control zone for an event subscription.

@JiaLiPassion
Copy link
Contributor Author

@devoto13, thank you for the information. It is the same idea, and built-in one is faster than runOutsideOfAngular because it will bypass zone totally.

@ericmartinezr
Copy link
Contributor

Wouldn't be a "true" or "false" be simpler? true for running inside angular's zone (on by default), and false for running outside angular's zone.

@JiaLiPassion
Copy link
Contributor Author

@ericmartinezr , yeah, true/false will be more clearer when we only need in ngZone or not.

I use string becasue there are other options, for example, the default, in fact, current @HostListener behavior is not in ngZone, it is default, that means, the event handler will run in the zone where it is created.

@JiaLiPassion JiaLiPassion force-pushed the hostzone branch 2 times, most recently from 32aec18 to 3fc9264 Compare January 21, 2018 13:44
@JiaLiPassion JiaLiPassion changed the title WIP(compiler): fix #19878, can config zone in HostListener decorator feat(compiler): fix #19878, can config zone in HostListener decorator Jan 21, 2018
@JiaLiPassion JiaLiPassion force-pushed the hostzone branch 4 times, most recently from ceb3984 to 62e59a4 Compare January 22, 2018 02:14
@mhevery mhevery self-requested a review January 22, 2018 20:41
@mhevery mhevery added the area: core Issues related to the framework runtime label Jan 22, 2018
@JiaLiPassion
Copy link
Contributor Author

I just saw the discussion here, #11200.
About

  • event (such as `<button (click)="onClick($event)">)
  • @HostListener

There are lot of options need to be extended in the future.

  • capture
  • passive
  • zone
  • ...

I am not sure which design is better.
I saw two designs have been discussed here.

  1. add a object options. just like the design here, [feat] Take advantage of passive event listeners #8866 (comment)
<button (click: {passive: true})="onClick($event)"></button>
  1. use a magic prefix. About the design here by @DzmitryShylovich UseCapture:true event handlers #11200 (comment)
~click => capture: true
-scroll=> passive: true
#scroll=> zone: ngzone
$scroll=> zone: nozone

I prefer the second one, the first one need user to write more code and will need to modify more codes of core/complier, the second one is more friendly.

@mhevery , please review, and I will try to modify this PR with the second one first to see is there any risk, thank you.

@mhevery
Copy link
Contributor

mhevery commented Jan 26, 2018

@mhevery
Copy link
Contributor

mhevery commented Jan 26, 2018

/cc @IgorMinar It would be great if you add your opinion.

@JiaLiPassion

  1. Your current PR is nice but it only solves it for @HostListener Using a prefix would solve it globally. I would much rather have a solution which would be global (for all kinds of listeners).
  2. I like the idea of encoding it into the string but the suggestion you have is that it does not differentiate between DOM events and component events. (For example if you have a component which has open output than capture, passive are meaningless.)
  3. We already have keydown.control.shift.enter listeners see https://github.com/angular/angular/blob/master/modules/playground/e2e_test/key_events/key_events_spec.ts and this feature is for DOM events only. Could we take this approach and extend it?

What I have in mind is something like this.

<div (click)="..."
     (click.nozone.passize.capture)="...">

I am open for suggestions as how to make it better... What do you think.

Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

I just skimmed through and I'm not sure yet if we want to do this at all, but if we do go forward, we should change the api to use an object literal config object rather than string literal. This would allow us to also support passive listeners (see #8866) and other options.

@IgorMinar
Copy link
Contributor

I'll review this in more detail next week, just wanted to provide quick feedback on the API.

@IgorMinar IgorMinar added the action: review The PR is still awaiting reviews from at least one requested reviewer label Jan 26, 2018
@mhevery
Copy link
Contributor

mhevery commented Jan 26, 2018

@IgorMinar I agree that current implementation is not what we want, but what about my proposal as described in the comment #21681 (comment)

@JiaLiPassion
Copy link
Contributor Author

@mhevery , @IgorMinar , thank you for the review.

We already have keydown.control.shift.enter listeners see https://github.com/angular/angular/blob/master/modules/playground/e2e_test/key_events/key_events_spec.ts and this feature is for DOM events only. Could we take this approach and extend it?
What I have in mind is something like this.

I am open for suggestions as how to make it better... What do you think.

Yes, you are right, I forget we already have the syntax like keydown.control.shift.enter, I agree

<div (click)="..."
    (click.nozone.passive.capture)="...">

is a better and consistent solution.

@JiaLiPassion JiaLiPassion changed the title feat(compiler): fix #19878, can config zone in HostListener decorator WIP(compiler): fix #19878, can config zone in HostListener decorator Mar 18, 2018
@JiaLiPassion JiaLiPassion changed the title WIP(compiler): fix #19878, can config zone in HostListener decorator feat(compiler): fix #19878, can config zone/once/passive/capture in template/HostListener decorator Mar 18, 2018
@JiaLiPassion
Copy link
Contributor Author

JiaLiPassion commented Mar 18, 2018

@mhevery , @IgorMinar , I have updated this PR, it is still WIP, please review it is ok or not, I will continue to add test cases and document. Thank you!

basically the options below can be configured.

  • EventListenerOptions
    • capture
    • passive
    • once
  • Zone Options
    • ngzone
    • nozone

And those options can be configured in

  • template
<div (mouseover.capture.once.passive)="mouseover();"></div>
  • HostListeners
 @HostListener('window:resize.nozone', ['$event.target'])
  onResize(target: any) {
    console.log('resize triggered');
  }

The order of capture/once/passive/nozone/ngzone is not relevant.
And it can also work with existing event syntax like keypress.ctrl.nozone.capture.

The current PR will have to wait for a PR of zone.js angular/zone.js#1053,
because IE not support addEventListener(eventName, listener, options) with the 3rd parameter to be an option object. in zone.js PR, I fix this issue for IE.

@JiaLiPassion JiaLiPassion changed the title feat(compiler): fix #19878, can config zone/once/passive/capture in template/HostListener decorator WIP(compiler): fix #19878, can config zone/once/passive/capture in template/HostListener decorator Mar 18, 2018
@ngbot
Copy link

ngbot bot commented Apr 2, 2018

Hi @JiaLiPassion! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@okhobb
Copy link

okhobb commented May 20, 2018

@JiaLiPassion Is it the case that it's assumed passive is false unless included in the list? Per https://bugs.webkit.org/show_bug.cgi?id=182521#c3 (causing issues like mapbox/mapbox-gl-js#6095 -- not an Angular project but illustrates why some apps are broken) it is now the case that it defaults to true on window and document events. Thus, users may explicitly need to configure it to be false in some cases.

@JiaLiPassion
Copy link
Contributor Author

@okhobb , thank you for the information, I will check it the situation when event such as touchstart is passive:true by default.

@pkozlowski-opensource pkozlowski-opensource added area: testing Issues related to Angular testing features, such as TestBed and removed area: core Issues related to the framework runtime labels Dec 19, 2018
@ngbot ngbot bot added this to the needsTriage milestone Dec 19, 2018
@pkozlowski-opensource pkozlowski-opensource added area: core Issues related to the framework runtime and removed area: testing Issues related to Angular testing features, such as TestBed labels Dec 19, 2018
@CollinGraf314
Copy link

This is something I would really like to be able to use. Is there any news on whether or not .nozone, .ngzone, and the like will implemented?

@ahnpnl
Copy link
Contributor

ahnpnl commented Jul 28, 2019

Hi,

Any new updates on this ? This feature is very nice and useful for developers.

@JiaLiPassion
Copy link
Contributor Author

Close this one and open a new PR #38236

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Aug 26, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants