-
Notifications
You must be signed in to change notification settings - Fork 80
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
PIA-1173: Add Bootstrapper and monitoring for VPN and Auth status #65
Conversation
2b28b70
to
68e240a
Compare
68e240a
to
38cfc76
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @kp-laura-sempere!
enum UserAuthenticationStatus { | ||
case loggedIn | ||
case loggedOut | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 callupdateState
and setisBootstrapped
to true right after callingbootstrap()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: