VagrantAI-c / ng-carousel-cdk

Configurable engine for carousel made with Angular Library
https://vagrantai-c.github.io/ng-carousel-cdk/
MIT License
12 stars 3 forks source link

All bound events trigger change detection #133

Closed montella1507 closed 1 year ago

montella1507 commented 2 years ago

All event bindings should be inside runOutsideAngularZone -> Then Jump back into angular zone only when neccessary

https://github.com/VagrantAI-c/ng-carousel-cdk/blob/development/projects/ng-carousel/src/lib/private/service/pan-recognizer.service.ts

Atm. every mouse drag triggers change detection and it is not neccessary in 99,9% of time.

https://user-images.githubusercontent.com/10418323/189343402-904ae562-9cdf-4964-b1db-f030d2be6226.mov

VagrantAI-c commented 1 year ago

Hello, @montella1507, and sorry for the late response.

Change detection is necessary because every drag iteration carousel recalculates its position and active slides. Even if events are to be run outside zone, drag recalculation would trigger change detection in any way.

Are there any performance issues related to this behavior?

montella1507 commented 1 year ago

Hello @VagrantAI-c ,

1) i dont think it is neccessary to trigger change detection to recalculate its position - not sure if you use change detection ? (ngStyle binding or something similiar) - even if you need that, you should not trigger tick() and you should use detectChanges() on local component instance. 2) You should trigger change detection (runInZone() only when Active slide changes - no need to trigger 10000000 change detections with every mouse move..) 3) Yes, there is performance issue related to this behavior, this is why you should bind mouseMove etc. outsideAngular Zone... You really dont want to trigger change detection in every animation frame / mouse move / scroll... it will shut the performance down.

https://netbasal.com/optimizing-angular-change-detection-triggered-by-dom-events-d2a3b2e11d87 https://stackoverflow.com/questions/43364463/angular-2-changedetection-and-mousemove-event

VagrantAI-c commented 1 year ago

Currently, there are no manual change detection actions: everything is handled by Angular. I can wrap these handlers outside zone, but then they'll return to zone anyways since there's a lot of recalculations for every tick (transform of the carousel, a11y attributes, template context recalculation). I did a little experiment and there're no changes whether event listener fires inside or outside zone.

The only thing I can meddle with is to detach zone completely and do all the recalculations manually. But I'm afraid that it might affect slide templates' CD in a bad way... Can try though.

VagrantAI-c commented 1 year ago

So I played a bit with manual change detection, and from this

Снимок экрана 2022-10-25 в 11 49 08

I got this

Снимок экрана 2022-10-25 в 12 41 32

I also plan to remove a11y and animations package and do less operations on slides differ

montella1507 commented 1 year ago

If there is triggered ChangeDetection in pointermove, there is probably not manual change detection :-/

You have to add event listener on pointermove outside angular zone (probably inside mousedown event handler).

Pointermove should not trigger change detection. Can you share your "demo" ?

VagrantAI-c commented 1 year ago

Alright, I think I got the point. The problem is within the slide array change. Every operation in carousel, like drag, index change, input change, resize, generates a new state. New state includes a new set of slides. It is filtered by trackBy function and therefore every state emit doesn't rerender slides, but triggers CD (manually or by async pipe). I guess I have to move this behavior from trackBy to the core, to clean functions itself.

So I have 18 functions which change the state, I have to traverse through them and add an additional flag of whether slides were really changed during the operation, also modify tests. Would take a while.

github-actions[bot] commented 1 year ago

:tada: This issue has been resolved in version 1.10.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket: