naver / egjs-infinitegrid

A module used to arrange card elements including content infinitely on a grid layout.
https://naver.github.io/egjs-infinitegrid/
MIT License
2.23k stars 95 forks source link

fix(ngx-infinitegrid): reduce change detection cycles and teardown event listeners once the view is removed #518

Closed arturovt closed 2 years ago

CLAassistant commented 2 years ago

CLA assistant check
All committers have signed the CLA.

malangfox commented 2 years ago

@arturovt I have a question about the changes made here and previous changes. Since using emit from EventEmitter causes change detection, it seems that firing event can cause change detection even after runOutsideAngular is applied. Is there any improvements other than using takeUntil for handling events? Maybe there's something I'm missing out?

arturovt commented 2 years ago

The takeUntil is only responsible for removing event listeners once the view is removed.

EventEmitter and emit don't cause change detections. Events are usually coming from other event listeners setup previously and other events are causing change detections. E.g.:

myButton.addEventListener('click', event => this.myEmitter.emit(event))

When creating a grid, in this case outside of the Angular zone, it remembers the Zone.current. The Zone.current is globally available and refers to the current execution context. E.g.:

declare const Zone: any;

class MyCoolComponent {
  constructor(ngZone: NgZone) {
    console.log(Zone.current); // angular
    ngZone.runOutsideAngular(() => console.log(Zone.current)); // <root> (the most parent)
  }
}

Which means any event setup by GridClass during the creation will remember the root zone (the execution context they were created in). When listening to grid events by grid.on they will be coming from the root zone context (because we created GridClass outside of the Angular zone). But it's necessary for any EventEmitter to trigger change detection because the user might be doing any kind of template code (e.g. updates some variable and it should be updated automatically).

But since there's no reason to trigger change detection unnecessarily I'm checking if there're any emitter listeners by emitter.observers.length > 0. Say we have an emitter @Output() renderComplete = new EventEmitter(). Its observers.length will be greater than zero when someone will setup a listener on the component <my-component (renderComplete)="onRenderComponent()"></my-component>.

The ngZone.run is basically a trigger for Angular like hey, there's some event happened, just run the change detection.