PIA-1173: Add Bootstrapper and monitoring for VPN and Auth status #65
Conversation
kp-said-rehouni
force-pushed
the
PIA-1173_bootstraper_tvos
branch
5 times, most recently
from
2b28b70
to
68e240a
Compare
kp-said-rehouni
force-pushed
the
PIA-1173_bootstraper_tvos
branch
from
68e240a
to
38cfc76
Compare
There was a problem hiding this comment.
Amazing job @kp-said-rehouni 🚀
I just added some minor questions, regarding some details. But nothing that blocks this from being merged ✅
| renewalDIPToken() | ||
| setupExceptionHandler() | ||
| } | ||
| } |
There was a problem hiding this comment.
Sweet!
Nice separation into different dependencies to perform all the bootstrap tasks (which are quite a lot 😅 ), but this implementation is easy to follow/update 👍
There was a problem hiding this comment.
Thank you @kp-laura-sempere!
| enum UserAuthenticationStatus { | ||
| case loggedIn | ||
| case loggedOut | ||
| } |
There was a problem hiding this comment.
We don't have to add them at this point, but probably later we would also have here things like expired or similar, right?
There was a problem hiding this comment.
Yes, correct. I'll create a ticket for it because we will also need to show a message to the users and probably navigate to a paywall screen in the future
| deinit { | ||
| notificationCenter.removeObserver(self) | ||
| } | ||
| } |
There was a problem hiding this comment.
I think this implementataon and the one VPNStatusMonitor look good and clear to me.
But there is also a way to subscribe to NotificationCenter directly from a Publisher and assign the value to a Combine Published too. I have not tested that approach myself yet.
But maybe for the future, we can explore that idea too.
These are some references:
https://developer.apple.com/documentation/foundation/notificationcenter/publisher
https://stackoverflow.com/questions/58559908/combine-going-from-notification-center-addobserver-with-selector-to-notificatio
Although to add Published properties to a protocol, we have to wrap them in another property. So it is a bit more of setup. Tbh, I really like your approach here and in the VPNStatusMonitor 👍
There was a problem hiding this comment.
That's a really good idea. I completely agree with you, although I just looked a bit into it and as you say, the setup requires some effort in order to not decouple the protocol with Combine. Maybe we can give it a try in the future. I'll create a ticket for that.
Thank you @kp-laura-sempere!
| // Bootstrap PIALibrary preferences and settings | ||
| // TODO: DI this object | ||
| Bootstrapper.shared.bootstrap() | ||
| bootstrap() | ||
| isBootstrapped = true | ||
| updateState() |
There was a problem hiding this comment.
The only question I have here is:
- Does
bootstrap()perform any async task that we have to wait for before updating the state? Or is it safe to callupdateStateand setisBootstrappedto true right after callingbootstrap()?
There was a problem hiding this comment.
That's a good question! It does not perform async tasks at this moment. I don't think that would change, but if it does we can update it.
Thank you|
Summary
This PR implements the following: