Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check null for subscriptions in ngOnDestroy #79

Closed
wants to merge 1 commit into from

Conversation

ravanjamjah
Copy link

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)

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)
@@ -325,7 +325,7 @@ export class KtdGridComponent implements OnChanges, AfterContentInit, AfterConte
}

ngOnDestroy() {
this.subscriptions.forEach(sub => sub.unsubscribe());
this.subscriptions?.forEach(sub => sub.unsubscribe());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about initializing subscriptions as an empty array?

private subscriptions: Subscription[] = []

Seems cleaner since it will always be an array of Subscriptions.

@llorenspujol
Copy link
Contributor

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
Copy link

Dji75 commented Apr 5, 2024

What is the status of this PR ?

I encounter the same issue

@JavadHS3
Copy link

JavadHS3 commented Apr 8, 2024

What is the status of this PR ?

I encounter the same issue

I fixed the problem in our project.

@Dji75
Copy link

Dji75 commented Apr 11, 2024

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
Copy link
Author

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
Copy link

Dji75 commented Apr 14, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants