-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat(ios): add SPM dependency resolution support alongside CocoaPods #8933
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
base: main
Are you sure you want to change the base?
Changes from all commits
05eea19
00f59b2
27dde8d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| require 'json' | ||
| require '../app/firebase_spm' | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maintainer note (mirrored in similar note in |
||
| package = JSON.parse(File.read(File.join(__dir__, 'package.json'))) | ||
| appPackage = JSON.parse(File.read(File.join('..', 'app', 'package.json'))) | ||
|
|
||
|
|
@@ -45,24 +46,41 @@ Pod::Spec.new do |s| | |
| end | ||
|
|
||
| # Firebase dependencies | ||
| s.dependency 'FirebaseAnalytics/Core', firebase_sdk_version | ||
| if defined?($RNFirebaseAnalyticsWithoutAdIdSupport) && ($RNFirebaseAnalyticsWithoutAdIdSupport == true) | ||
| Pod::UI.puts "#{s.name}: Not installing FirebaseAnalytics/IdentitySupport Pod, no IDFA will be collected." | ||
| # Analytics has conditional dependencies that vary between SPM and CocoaPods. | ||
| # SPM: use FirebaseAnalyticsWithoutAdIdSupport when $RNFirebaseAnalyticsWithoutAdIdSupport = true | ||
| # to avoid GoogleAppMeasurement APM symbols that require FirebaseRemoteConfig (linker error). | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. was it FirebasePerformance symbols or FirebaseRemoteConfig symbols missing at link time? The comment on the PR and the comment here in code and slightly lower in code are inconsistent 🤔 |
||
| # CocoaPods: IdentitySupport is a separate subspec controlled by $RNFirebaseAnalyticsWithoutAdIdSupport. | ||
| if defined?(spm_dependency) && !defined?($RNFirebaseDisableSPM) && | ||
| defined?($RNFirebaseAnalyticsWithoutAdIdSupport) && $RNFirebaseAnalyticsWithoutAdIdSupport | ||
| # FirebaseAnalyticsCore uses GoogleAppMeasurementCore (no IDFA, no APM objects). | ||
| # FirebaseAnalytics uses GoogleAppMeasurement which has APMETaskManager/APMMeasurement | ||
| # cross-references that cause linker errors when FirebasePerformance is not linked. | ||
| Pod::UI.puts "#{s.name}: Using FirebaseAnalyticsCore SPM product (no IDFA, uses GoogleAppMeasurementCore)." | ||
| firebase_dependency(s, firebase_sdk_version, ['FirebaseAnalyticsCore'], 'FirebaseAnalytics/Core') | ||
| else | ||
| if !defined?($RNFirebaseAnalyticsWithoutAdIdSupport) | ||
| Pod::UI.puts "#{s.name}: Using FirebaseAnalytics/IdentitySupport with Ad Ids. May require App Tracking Transparency. Not allowed for Kids apps." | ||
| Pod::UI.puts "#{s.name}: You may set variable `$RNFirebaseAnalyticsWithoutAdIdSupport=true` in Podfile to use analytics without ad ids." | ||
| end | ||
| s.dependency 'FirebaseAnalytics/IdentitySupport', firebase_sdk_version | ||
| firebase_dependency(s, firebase_sdk_version, ['FirebaseAnalytics'], 'FirebaseAnalytics/Core') | ||
| end | ||
|
|
||
| # Special pod for on-device conversion | ||
| if defined?($RNFirebaseAnalyticsEnableAdSupport) && ($RNFirebaseAnalyticsEnableAdSupport == true) | ||
| Pod::UI.puts "#{s.name}: Adding Apple AdSupport.framework dependency for optional analytics features" | ||
| s.frameworks = 'AdSupport' | ||
| unless defined?(spm_dependency) | ||
| # CocoaPods-only: conditional IdentitySupport subspec | ||
| if defined?($RNFirebaseAnalyticsWithoutAdIdSupport) && ($RNFirebaseAnalyticsWithoutAdIdSupport == true) | ||
| Pod::UI.puts "#{s.name}: Not installing FirebaseAnalytics/IdentitySupport Pod, no IDFA will be collected." | ||
| else | ||
| if !defined?($RNFirebaseAnalyticsWithoutAdIdSupport) | ||
| Pod::UI.puts "#{s.name}: Using FirebaseAnalytics/IdentitySupport with Ad Ids. May require App Tracking Transparency. Not allowed for Kids apps." | ||
| Pod::UI.puts "#{s.name}: You may set variable `$RNFirebaseAnalyticsWithoutAdIdSupport=true` in Podfile to use analytics without ad ids." | ||
| end | ||
| s.dependency 'FirebaseAnalytics/IdentitySupport', firebase_sdk_version | ||
| end | ||
| end | ||
|
|
||
| # Special pod for on-device conversion | ||
| # AdSupport framework (works with both SPM and CocoaPods) | ||
| if defined?($RNFirebaseAnalyticsEnableAdSupport) && ($RNFirebaseAnalyticsEnableAdSupport == true) | ||
| Pod::UI.puts "#{s.name}: Adding Apple AdSupport.framework dependency for optional analytics features" | ||
| s.frameworks = 'AdSupport' | ||
| end | ||
|
|
||
| # GoogleAdsOnDeviceConversion (CocoaPods only, not available in firebase-ios-sdk SPM) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there some documentation on how to access on-device conversion in the SPM case? This is surprising to me as I thought on-device conversion was one of the newer features of firebase-ios-sdk which creates the expectation in me that it should be available there somehow ? Perhaps just built in to core SPM dep I'm not sure |
||
| if defined?($RNFirebaseAnalyticsGoogleAppMeasurementOnDeviceConversion) && ($RNFirebaseAnalyticsGoogleAppMeasurementOnDeviceConversion == true) | ||
| Pod::UI.puts "#{s.name}: GoogleAdsOnDeviceConversion pod added" | ||
| s.dependency 'GoogleAdsOnDeviceConversion' | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,7 +15,12 @@ | |
| * | ||
| */ | ||
|
|
||
| #if __has_include(<Firebase/Firebase.h>) | ||
| #import <Firebase/Firebase.h> | ||
| #else | ||
| @import FirebaseCore; | ||
| @import FirebaseAnalytics; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Taking note of your comment here on the FirebasePerformance symbols missing at link time if ad ids are disabled does this |
||
| #endif | ||
| #import <React/RCTUtils.h> | ||
|
|
||
| #if __has_include(<RNFBAnalytics/RNFBAnalytics-Swift.h>) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,8 +15,15 @@ | |
| * | ||
| */ | ||
|
|
||
| #if __has_include(<Firebase/Firebase.h>) | ||
| #import <Firebase/Firebase.h> | ||
| #import <FirebaseAppCheck/FIRAppCheck.h> | ||
| #elif __has_include(<FirebaseAppCheck/FirebaseAppCheck.h>) | ||
| #import <FirebaseAppCheck/FirebaseAppCheck.h> | ||
|
Comment on lines
+18
to
+21
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| #import <FirebaseCore/FirebaseCore.h> | ||
| #else | ||
| @import FirebaseCore; | ||
| @import FirebaseAppCheck; | ||
| #endif | ||
|
|
||
| #import <React/RCTUtils.h> | ||
|
|
||
|
|
@@ -53,7 +60,8 @@ + (instancetype)sharedInstance { | |
| : (BOOL)isTokenAutoRefreshEnabled | ||
| : (RCTPromiseResolveBlock)resolve rejecter | ||
| : (RCTPromiseRejectBlock)reject) { | ||
| DLog(@"deprecated API, provider will be deviceCheck / token refresh %d for app %@", | ||
| DLog(@"deprecated API, provider will be deviceCheck / token refresh %d for " | ||
| @"app %@", | ||
|
Comment on lines
-56
to
+64
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changing spacing unrelated to functional changes is not recommended, it's enforced with a project-wide config and a lint check via |
||
| isTokenAutoRefreshEnabled, firebaseApp.name); | ||
| [[RNFBAppCheckModule sharedInstance].providerFactory configure:firebaseApp | ||
| providerName:@"deviceCheck" | ||
|
|
@@ -86,8 +94,8 @@ + (instancetype)sharedInstance { | |
| appCheck.isTokenAutoRefreshEnabled = isTokenAutoRefreshEnabled; | ||
| } | ||
|
|
||
| // Not present in JS or Android - it is iOS-specific so we only call this in testing - it is not in | ||
| // index.d.ts | ||
| // Not present in JS or Android - it is iOS-specific so we only call this in | ||
| // testing - it is not in index.d.ts | ||
| RCT_EXPORT_METHOD(isTokenAutoRefreshEnabled | ||
| : (FIRApp *)firebaseApp | ||
| : (RCTPromiseResolveBlock)resolve rejecter | ||
|
|
@@ -105,33 +113,36 @@ + (instancetype)sharedInstance { | |
| : (RCTPromiseRejectBlock)reject) { | ||
| FIRAppCheck *appCheck = [FIRAppCheck appCheckWithApp:firebaseApp]; | ||
| DLog(@"appName %@", firebaseApp.name); | ||
| [appCheck | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same comment on spacing - I won't repeat it for others, but in general never change spacing unrelated to the PR, it creates larger / less relevant diffs and will need a revert as it will fail lint anyway |
||
| tokenForcingRefresh:forceRefresh | ||
| completion:^(FIRAppCheckToken *_Nullable token, NSError *_Nullable error) { | ||
| if (error != nil) { | ||
| // Handle any errors if the token was not retrieved. | ||
| DLog(@"RNFBAppCheck - getToken - Unable to retrieve App Check token: %@", error); | ||
| [RNFBSharedUtils rejectPromiseWithUserInfo:reject | ||
| userInfo:(NSMutableDictionary *)@{ | ||
| @"code" : @"token-error", | ||
| @"message" : [error localizedDescription], | ||
| }]; | ||
| return; | ||
| } | ||
| if (token == nil) { | ||
| DLog(@"RNFBAppCheck - getToken - Unable to retrieve App Check token."); | ||
| [RNFBSharedUtils rejectPromiseWithUserInfo:reject | ||
| userInfo:(NSMutableDictionary *)@{ | ||
| @"code" : @"token-null", | ||
| @"message" : @"no token fetched", | ||
| }]; | ||
| return; | ||
| } | ||
|
|
||
| NSMutableDictionary *tokenResultDictionary = [NSMutableDictionary new]; | ||
| tokenResultDictionary[@"token"] = token.token; | ||
| resolve(tokenResultDictionary); | ||
| }]; | ||
| [appCheck tokenForcingRefresh:forceRefresh | ||
| completion:^(FIRAppCheckToken *_Nullable token, NSError *_Nullable error) { | ||
| if (error != nil) { | ||
| // Handle any errors if the token was not retrieved. | ||
| DLog(@"RNFBAppCheck - getToken - Unable to retrieve App " | ||
| @"Check token: %@", | ||
| error); | ||
| [RNFBSharedUtils | ||
| rejectPromiseWithUserInfo:reject | ||
| userInfo:(NSMutableDictionary *)@{ | ||
| @"code" : @"token-error", | ||
| @"message" : [error localizedDescription], | ||
| }]; | ||
| return; | ||
| } | ||
| if (token == nil) { | ||
| DLog(@"RNFBAppCheck - getToken - Unable to retrieve App " | ||
| @"Check token."); | ||
| [RNFBSharedUtils rejectPromiseWithUserInfo:reject | ||
| userInfo:(NSMutableDictionary *)@{ | ||
| @"code" : @"token-null", | ||
| @"message" : @"no token fetched", | ||
| }]; | ||
| return; | ||
| } | ||
|
|
||
| NSMutableDictionary *tokenResultDictionary = [NSMutableDictionary new]; | ||
| tokenResultDictionary[@"token"] = token.token; | ||
| resolve(tokenResultDictionary); | ||
| }]; | ||
| } | ||
|
|
||
| RCT_EXPORT_METHOD(getLimitedUseToken | ||
|
|
@@ -140,32 +151,35 @@ + (instancetype)sharedInstance { | |
| : (RCTPromiseRejectBlock)reject) { | ||
| FIRAppCheck *appCheck = [FIRAppCheck appCheckWithApp:firebaseApp]; | ||
| DLog(@"appName %@", firebaseApp.name); | ||
| [appCheck limitedUseTokenWithCompletion:^(FIRAppCheckToken *_Nullable token, | ||
| NSError *_Nullable error) { | ||
| if (error != nil) { | ||
| // Handle any errors if the token was not retrieved. | ||
| DLog(@"RNFBAppCheck - getLimitedUseToken - Unable to retrieve App Check token: %@", error); | ||
| [RNFBSharedUtils rejectPromiseWithUserInfo:reject | ||
| userInfo:(NSMutableDictionary *)@{ | ||
| @"code" : @"token-error", | ||
| @"message" : [error localizedDescription], | ||
| }]; | ||
| return; | ||
| } | ||
| if (token == nil) { | ||
| DLog(@"RNFBAppCheck - getLimitedUseToken - Unable to retrieve App Check token."); | ||
| [RNFBSharedUtils rejectPromiseWithUserInfo:reject | ||
| userInfo:(NSMutableDictionary *)@{ | ||
| @"code" : @"token-null", | ||
| @"message" : @"no token fetched", | ||
| }]; | ||
| return; | ||
| } | ||
|
|
||
| NSMutableDictionary *tokenResultDictionary = [NSMutableDictionary new]; | ||
| tokenResultDictionary[@"token"] = token.token; | ||
| resolve(tokenResultDictionary); | ||
| }]; | ||
| [appCheck | ||
| limitedUseTokenWithCompletion:^(FIRAppCheckToken *_Nullable token, NSError *_Nullable error) { | ||
| if (error != nil) { | ||
| // Handle any errors if the token was not retrieved. | ||
| DLog(@"RNFBAppCheck - getLimitedUseToken - Unable to retrieve App Check " | ||
| @"token: %@", | ||
| error); | ||
| [RNFBSharedUtils rejectPromiseWithUserInfo:reject | ||
| userInfo:(NSMutableDictionary *)@{ | ||
| @"code" : @"token-error", | ||
| @"message" : [error localizedDescription], | ||
| }]; | ||
| return; | ||
| } | ||
| if (token == nil) { | ||
| DLog(@"RNFBAppCheck - getLimitedUseToken - Unable to retrieve App Check " | ||
| @"token."); | ||
| [RNFBSharedUtils rejectPromiseWithUserInfo:reject | ||
| userInfo:(NSMutableDictionary *)@{ | ||
| @"code" : @"token-null", | ||
| @"message" : @"no token fetched", | ||
| }]; | ||
| return; | ||
| } | ||
|
|
||
| NSMutableDictionary *tokenResultDictionary = [NSMutableDictionary new]; | ||
| tokenResultDictionary[@"token"] = token.token; | ||
| resolve(tokenResultDictionary); | ||
| }]; | ||
| } | ||
|
|
||
| @end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,8 +15,15 @@ | |
| * | ||
| */ | ||
|
|
||
| #if __has_include(<Firebase/Firebase.h>) | ||
| #import <Firebase/Firebase.h> | ||
| #import <FirebaseAppCheck/FIRAppCheck.h> | ||
| #elif __has_include(<FirebaseAppCheck/FirebaseAppCheck.h>) | ||
| #import <FirebaseAppCheck/FirebaseAppCheck.h> | ||
| #import <FirebaseCore/FirebaseCore.h> | ||
|
Comment on lines
+18
to
+22
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same questions on imports here - I note that Firebase vs FirebaseCore varies here though - I may have missed that above and that might explain the entire structure |
||
| #else | ||
| @import FirebaseCore; | ||
| @import FirebaseAppCheck; | ||
| #endif | ||
|
|
||
| @interface RNFBAppCheckProvider : NSObject <FIRAppCheckProvider> | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -68,3 +68,6 @@ android/.settings | |
| type-test.ts | ||
| scripts | ||
| __tests__ | ||
|
|
||
| # Force include generated version file (needed for linking) | ||
| !ios/RNFBApp/RNFBVersion.m | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this be added to the files array in |
||
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 prefer the variable interpolations at the front and all together and the various naming / key "sauce" at the end