Check null for subscriptions in ngOnDestroy

ravanjamjah commented 1 year ago

In a scenario I got below error: core.mjs:6397 ERROR TypeError: Cannot read properties of undefined (reading 'forEach') at KtdGridComponent.ngOnDestroy (katoid-angular-grid-layout.mjs:1612:28)

llorenspujol commented 1 year ago

Nice @ravanjamjah 👌 .

I added a small comment proposing initializing this.subscriptions as empty array.

It will also be nice if you change the commit text to conventional commits. For example:

fix(grid): initialize subscriptions to empty array

Fixes the error 'TypeError: Cannot read properties of undefined (reading 'forEach')
at KtdGridComponent.ngOnDestroy' that can happen in some specific scenarios.

Thanks!

Dji75 commented 3 months ago

What is the status of this PR ?

I encounter the same issue

JavadHS3 commented 3 months ago

What is the status of this PR ?

I encounter the same issue

I fixed the problem in our project.

Dji75 commented 3 months ago

What is the status of this PR ? I encounter the same issue

I fixed the problem in our project.

Glad for you ;)

On our side, I was able to workrounded the issue by removing a *ngIf in the hierarchy of the components displaying the grid (in a parent in fact), but anyway this could bore anyone who can't workaround it :)

ravanjamjah commented 3 months ago

Thanks, exactly as I did, I fixed with a *ngIf. I think this happens because the constructor cannot fully initialize the object. for that reason i suggested using ?. operator. we can investigate more but need more testing.

Dji75 commented 3 months ago

Maybe.

How to reproduce the problem:

ktgrid is used in a component A which is lazy loaded (standalone: true, changeDetection: ChangeDetectionStrategy.OnPush), no logic inside, just display static data.
There is also a component B with a router-outlet but due to functional complexity, it had previously 2 router-outlet : one inside a navigation panel for some kind of urls, another one (alone) for some another kind of urls (component A with our grid is displayed here)

In the component B, depending of the urls, there were a *ngIf to display (at the end) the right router-outlet and (at the end) the right component.

It was not a correct design for routing (even if it would work as is) so I fully rework this and it work around the grid issue.

The new routing completely get rid of any display logic (the *ngIf) and to only use lazy loading to display components, so I only keep one router-outlet with different component's routing (some routes with a wrapper including navigation panel, another routing for our component A).

Hope it can helps

katoid / angular-grid-layout

Check null for subscriptions in ngOnDestroy #79