ngx-lite icon indicating copy to clipboard operation
ngx-lite copied to clipboard

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

Open kevinbuhmann opened this issue 5 years ago • 1 comments

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.

kevinbuhmann avatar Nov 26 '19 21:11 kevinbuhmann

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.

coryrylan avatar Nov 30 '19 19:11 coryrylan