search

GridtNetwork / gridt-client

Official client of the GridtNetwork.
https://gridt.org
Other
3 stars 1 forks source link

135 subscription audit #174

Open HendrikPreuter opened 3 years ago

HendrikPreuter commented 3 years ago

135

To-do: -Awaiting round 2 of feedback

Done:

Drvanon commented 3 years ago

Also, on the services and app.component, onDestroy will never be called. Sorry for not mentioning this earlier, but the page will just be destroyed. There are places where it might make sense to put time outs, if a subscription is performed in a function and then never looked after again. If the time out happens, it should however throw a publicly visible error.

HendrikPreuter commented 3 years ago

I notice you tend to go for registering individual subscriptions and unsubcribing from them. Because the principle of unsubscribing is generally applicable and there is no further use for the subscriptions, it makes sense to gather them into one list and remove that list at the onDestroy.

Does that mean i still need to unsubscribe and then empty the list? Or just empty the list of subscriptions, because i feel that doesn't end them rightly.

Also, on the services and app.component, onDestroy will never be called. Sorry for not mentioning this earlier, but the page will just be destroyed. There are places where it might make sense to put time outs, if a subscription is performed in a function and then never looked after again. If the time out happens, it should however throw a publicly visible error.

We had a conversation about this and i remembered the part where your said a service gets terminated with the component it is attached to. However i forgot the only component they are attached to is in fact app.component. We also somewhat talked about these timeouts. You wanted something in sec storage right? WIth the takeuntil(5) or something along those lines. However i might still need some more training in rxjs for this as i only found 1 of the functions where i could put the .pipe(takeUntil(timer(5000))). And i was not even sure that one was the right spot.

Is what you are suggesting here something else or is it what i mentioned above?

Drvanon commented 3 years ago

The most important thing is that you unsubscribe onDestroy. The garbage collector can then itself determine what it will do with the subscription after the component is destroyed.

.pipe(takeUntil(timer(5000))). is indeed what you are looking for. I don't know if 5 seconds is the right time (haven't given it much thought), but that would make sure it doesn't crash. However, it would be good for debugging purposes, if you throw an error outside of the pipe (aka in the subscription), because nothing should fail silently.

Drvanon commented 3 years ago

Please make sure the checks pass before requesting a review. Also if you are still working on adding work to a pull request, please turn the pull request into a draft so the CI is not run all the time.

HendrikPreuter commented 3 years ago

Please make sure the checks pass before requesting a review. Also if you are still working on adding work to a pull request, please turn the pull request into a draft so the CI is not run all the time.

i will, that's why i have not re-requested feedback for now. I had an issue with merging again. Still trying to untangle that mess.