MurhafSousli / ngx-scrollbar

Custom overlay-scrollbars with native scrolling mechanism
https://ngx-scrollbar.netlify.app/
MIT License
612 stars 99 forks source link

Memory leaks with MatTable and externalViewport #611

Closed UnsightWu closed 1 month ago

UnsightWu commented 3 months ago

Reproduction

I'm trying to use ng-scrollbar with Angular MatTable. There is no possibility to use ng-scrollbar inside of table (it just being removed by material), so I used externalViewport approach:

` <mat-table [dataSource]="[]" scrollViewport

@for (col of columns; track col.key) { ... } `

In UI everything works properly, unfortunately this configuration leads to memory leaks. Angular stacks component instances each time I revisit route with it.

Steps to reproduce:

  1. Create component with mat-table.
  2. Wrap mat table with ng-scrollbar.
  3. Add externalViewport to ng-scrollbar.
  4. Use scrollViewport on "mat-table" component.
  5. Try to destroy/create component few times (without page reload).
  6. Check inspector memory tab to detect multiple component instances.

Expected Behavior

No memory leaks for this configuration.

Actual Behavior

externalViewport + scrollViewport on mat-table cause memory leaks

Environment

MurhafSousli commented 3 months ago

Please add a stackblitz reproduction to show the memory leak.

Btw you don't need to set an external viewport, here is a material table example

UnsightWu commented 3 months ago

yeah, example you've provided doesn't work for me. I use sticky header for mat table, it is always placed on top, so if I wrap whole table with scroll it won't work.

UnsightWu commented 3 months ago

https://stackblitz.com/edit/stackblitz-starters-rkta8g?file=src%2Fapp%2Fapp.component.ts

here is my example. Each time you trigger table you get +1 table instance in memory. If you remove externalViewport + scrollViewport directives from code - tables wont stack.

MurhafSousli commented 3 months ago

I tried it and went to the memory tab, but I still don't see any duplication, could you share a screenshot or video recording!

UnsightWu commented 3 months ago

Sure, for some reason stackblitz doesn't recognize MatTable class properly in debug tool. Try to download project from stackblitz and run locally to have same result as mine.

with externalViewport https://github.com/MurhafSousli/ngx-scrollbar/assets/11653542/1a5c0b22-d980-40f9-a3de-c7e981fca52a

without externalViewport https://github.com/MurhafSousli/ngx-scrollbar/assets/11653542/b329d0c0-7985-43e1-adc6-82b99640a2d0

MurhafSousli commented 1 month ago

Update

After investigation, I found the following:

Toggling on and off the mat-table component without wrapping it in the scrollbar component, show duplicate instance _MatTable×2.

Toggling on and off the mat-table component wrapped inside the scrollbar component, show duplicate instance _MatTable×2. (same result as without scrollbar)

Toggling on and off the mat-table component wrapped inside the scrollbar component and setting scrollViewport directive on mat-table component, show duplicate instance _MatTable×7.

The same occurs with externalViewport class method.

So the problem only occurs when scrollViewport is used on mat-table component.

This is pretty weird because the scrollViewport directive is dump and has no logic at all.

Solution

The quick solution is to use externalViewport withasyncDetection directive.

<ng-scrollbar externalViewport="mat-table" asyncDetection>
     <table mat-table [dataSource]="dataSource" class="mat-elevation-z8">
</ng-scrollbar>

I am still investigating the issue

MurhafSousli commented 1 month ago

The memory leak is fixed in v15.1.3.

NOTE: <table mat-table> isn't designed to be a scrollable element, you need to wrap it inside a scrollable container, or use the flex material table.