-
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 3 commits
05eea19
00f59b2
27dde8d
7b431cb
496d97c
392258f
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
Author
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 monorepo/pnpm note applies here. The |
||
| 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 🤔
Author
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. Fixed in latest commit. It was FirebasePerformance (APM = App Performance Monitoring). The symbols |
||
| # 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 | ||
|
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. IdentitySupport skipped when SPM disabled on RN >= 0.75High Severity The Reviewed by Cursor Bugbot for commit 496d97c. Configure here. |
||
| 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
Author
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. GoogleAdsOnDeviceConversion is a static xcframework distributed separately from firebase-ios-sdk. It is NOT available as an SPM product in In the latest commit I've added:
If firebase-ios-sdk adds it as an SPM product in the future, we can remove this restriction. |
||
| 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
Author
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. Good question. Yes, |
||
| #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.
Author
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. You're right that The three-branch structure is: CocoaPods umbrella, then CocoaPods individual pods, then SPM ( |
||
| #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 %@", | ||
|
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
Author
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. Reverted all unrelated spacing changes in the latest commit. The diff now only shows the |
||
| 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
Author
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 fix, all unrelated spacing reverted in latest commit. |
||
| 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
Author
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. Good catch on Firebase vs FirebaseCore. The difference is intentional: AppCheck needs both |
||
| #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
Author
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. Good point. This package doesn't use a |
||


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
Uh oh!
There was an error while loading. Please reload this page.
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.
Done, reordered in the latest commit.