Skip to content
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

Auth integration with App Multicast Delegate #8422

Closed
wants to merge 8 commits into from

Conversation

maksymmalyhin
Copy link
Contributor

b/193554796

@google-oss-bot
Copy link

1 Warning
⚠️ Did you forget to add a changelog entry? (Add #no-changelog to the PR description to silence this warning.)

Generated by 🚫 Danger

@@ -77,7 +77,14 @@
#import "FirebaseAuth/Sources/SystemService/FIRAuthAppCredentialManager.h"
#import "FirebaseAuth/Sources/SystemService/FIRAuthNotificationManager.h"
#import "FirebaseAuth/Sources/Utilities/FIRAuthURLPresenter.h"
#endif

#if SWIFT_PACKAGE
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Structure public headers as documented in https://github.com/firebase/firebase-ios-sdk/blob/master/HeadersImports.md#header-file-types-and-locations---for-header-file-creators to avoid need for SPM/CocoaPods specific imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, not sure I understand how it's applicable here because there are no headers in the lib currently, but only a single Swift file. But probably it won't be relevant any more after google/GoogleUtilities#50 (comment) since we will have to re-implement the multicast delegate in Objective-C.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right. I made this comment before I saw the implementation was Swift.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: Validate in Firebase

@@ -153,7 +153,7 @@ let package = Package(
.package(
name: "GoogleUtilities",
url: "https://github.com/google/GoogleUtilities.git",
"7.4.0" ..< "8.0.0"
.branch("mm/app-delegate-sample")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not merge to master.

@@ -482,7 +489,14 @@ - (nullable instancetype)initWithAPIKey:(NSString *)APIKey appName:(NSString *)a
}
UIApplication *application = [applicationClass sharedApplication];

[GULAppDelegateSwizzler proxyOriginalDelegateIncludingAPNSMethods];
// Use Multicast App Delegate when available but fallback to App Delegate Swizzler for backward compatibility.
id<GULMulticastAppDelegateProtocol> multicastDelegate =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this in iOS specific code?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess is it's due to phone auth not being available on other platforms, but that was out of ease-of-implementation not any product decisions.

id<GULMulticastAppDelegateProtocol> multicastDelegate =
[GULMulticastAppDelegate installedMulticastDelegate];
if (multicastDelegate == nil) {
FIRLogInfo(kFIRLoggerAuth, @"I-AUT000018", @"Multicast App delegate was not detected. Make sure app events forwarding is configured manually for you app. <Add link to docs>");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: long line

@paulb777
Copy link
Member

Closing since we no longer plan to do the multicast delegate

@paulb777 paulb777 closed this Dec 22, 2022
@firebase firebase locked and limited conversation to collaborators Jan 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants