[v11] Replace Auth worker queue with an actor - checkpoint WIP#13246
[v11] Replace Auth worker queue with an actor - checkpoint WIP#13246paulb777 wants to merge 7 commits into
Conversation
| forName: UIApplication.didBecomeActiveNotification, | ||
| object: nil, queue: nil | ||
| ) { notification in | ||
| self.isAppInBackground = false |
There was a problem hiding this comment.
This is ported unchanged from the old PR, but why not use applicationState instead of manually tracking the state?
There was a problem hiding this comment.
Looks like it would be a good improvement to eliminate self.isAppInBackground, but outside the scope of this PR.
Generated by 🚫 Danger |
| #if canImport(UIKit) // Is a similar mechanism needed on macOS? | ||
| applicationDidBecomeActiveObserver = | ||
| NotificationCenter.default.addObserver( | ||
| forName: UIApplication.didBecomeActiveNotification, |
There was a problem hiding this comment.
I'm wondering what happen with app extensions, in this way I think UIApplication.didBecomeActiveNotification and didEnterBackgroundNotification would not be fired @paulb777
There was a problem hiding this comment.
Good question. This implementation should match the current Objective-C one. Do you see an issue?
There was a problem hiding this comment.
Sure, even the didBecomeActiveNotification is available on extensions (imported via UIKit) and the other one as well, it won't be fired. so I would check if it is an extension or not using for example the following code:
var isRunningInExtension: Bool {
// Check if the bundle path ends with `.appex` (for iOS)
let bundlePath = Bundle.main.bundlePath
let isBundlePathExtension = bundlePath.hasSuffix(".appex")
// Check if the `NSExtension` key exists in the `Info.plist`
let isNSExtensionPresent = (Bundle.main.infoDictionary?["NSExtension"] as? [String: Any]) != nil
return isBundlePathExtension || isNSExtensionPresent
}
And In case of being true (the output) try to make a check in other way. Dispatch Queues with async after a time don't work on certain extensions as well as timers ... (Because apple timers and dispatch async after tend to sleep when the device is locked or with no activity for a long time) So you can take another approach like going with a counter with the system clock or infinite loop. They are not good approaches but extensions are very restricted. My suggestion in this case is surround with an if the observers and delegate in developers to run any kinda code, calling a func, when activity is registered on their extensions, in this way you won't impact performance in extensions.
| } | ||
|
|
||
| /// Configures Firebase Auth to connect to an emulated host instead of the remote backend. | ||
| @objc open func useEmulator(withHost host: String, port: Int) { |
There was a problem hiding this comment.
I think this code shouldn't be on a production framework cause it won't happen. Is there any way get rid of it without removing the ability to use it whereas developers are coding or testing on pre-production environments?
There was a problem hiding this comment.
It could be possible, but I'm not convinced the usability impact would be worth the relatively small size overhead increase.
There was a problem hiding this comment.
Sure, maybe an approach like surrounding the function with a comment (one above and one to close it) or any kinda special macro compiler for those who wants to remove it from production could make the trick. Who wants to remove it from prod, could build the frameworks by itself.
paulb777
left a comment
paulb777
left a comment
There was a problem hiding this comment.
@jesus-mg-ios Thanks for taking a look!
| #if canImport(UIKit) // Is a similar mechanism needed on macOS? | ||
| applicationDidBecomeActiveObserver = | ||
| NotificationCenter.default.addObserver( | ||
| forName: UIApplication.didBecomeActiveNotification, |
There was a problem hiding this comment.
Good question. This implementation should match the current Objective-C one. Do you see an issue?
| } | ||
|
|
||
| /// Configures Firebase Auth to connect to an emulated host instead of the remote backend. | ||
| @objc open func useEmulator(withHost host: String, port: Int) { |
There was a problem hiding this comment.
It could be possible, but I'm not convinced the usability impact would be worth the relatively small size overhead increase.
Anytime |
|
Parts of this PR have been implemented in other PRs. The bulk of moving the auth worker to an actor may need to wait for a breaking change release since public APIs doing dispatch_sync's may need to be converted to async APIs - or other alternatives like part of initialization. Marking to be considered with Firebase 12 |
5 participants
WIP checkpoint