codediodeio / angular-firestarter

🍱 :fire: Angular + Firebase Progressive Web App Starter
https://firestarter.fireship.io/
959 stars 438 forks source link

Potential memory leaks in auth.service.ts #4

Closed jsitu closed 6 years ago

jsitu commented 6 years ago

In the constructor, the authState is being subscribed in order to update the local authState variable:

constructor(private afAuth: AngularFireAuth,
        private db: AngularFireDatabase,
        private router:Router) {

        this.afAuth.authState.subscribe((auth) => {
        this.authState = auth
    });
}

I think there should be a unsubscription somewhere in the code for unhooking the authState so that every time auth.service gets injected it won't cause potential memory leaks. Potential solution:

authSubs = this.afAuth.authState.subscribe((auth) => {
    this.authState = auth
});

ngOnDestroy() {
    this.authSubs.unsubscribe();
}
codediodeio commented 6 years ago

This is a good suggestion. I'm working on some general updates, should have this issue updated in about a week.

codediodeio commented 6 years ago

After considering some changes, I'm going to leave this authState as-is. Firebase auth does not emit a memory-intensive stream of data, so it should't be a memory leak risk in a service singleton. Ending the subscription is simple if needed, but I think its unnecessary for this simple project.