coryrylan / ngx-lite

:package: A collection of lightweight Angular libraries in a single mono repo
https://ngxlite.com
MIT License
138 stars 21 forks source link

certain components cause page performance issues by using separate event listeners for global events #29

Open kevinbuhmann opened 4 years ago

kevinbuhmann commented 4 years ago

Describe the bug If you have several instances of ngx-menu on a page (e.g. in a row for each entity in a list), each one attaches an event listener for document:click and window:keyup. This can cause serious performance issues (slow response to clicks and typing in the browser).

To Reproduce https://stackblitz.com/edit/angular-vwdj4h

Notice that typing in the text box is really slow with a lot of menus on the page.

Expected behavior This should not happen. We fixed it by forking the menu component and making it use a shared observable for the document:click and window:keyup events instead of HostListener. This should be done for any component that uses HostListener for a global browser event if a page may contain several instances of that component. The datepicker, input tag, menu, and modal components in ngx-lite could suffer from this same issue.

This could be solved by using a SharedEventService like this:

import { Injectable, Inject, PLATFORM_ID } from '@angular/core';
import { fromEvent, EMPTY, Observable } from 'rxjs';
import { shareReplay } from 'rxjs/operators';
import { isPlatformBrowser } from '@angular/common';

@Injectable({ providedIn: 'root' })
export class SharedEventsService {
  readonly documentClickEvent: Observable<MouseEvent>;

  constructor(@Inject(PLATFORM_ID) platformId: string) {
    const isBrowser = isPlatformBrowser(platformId);

    this.documentClickEvent = isBrowser ? fromEvent<MouseEvent>(document, 'click').pipe(shareReplay()) : EMPTY;
  }
}

I was going to open a PR to fix all of the affected components, but I am unsure how to update @ngx-lite/util to add the shared service.

coryrylan commented 4 years ago

Hmm yeah this is a little tricky because each component is its own entrypoint. I am not sure if for root will resolve from separate modules. I'll have to make a test to see.