Repository navigation
Conversation
9ce2c9a to
01b2988
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9337 +/- ##
============================================
+ Coverage 70.00% 72.00% +2.00%
- Complexity 2134 2171 +37
============================================
Files 550 576 +26
Lines 38877 39862 +985
Branches 5301 5252 -49
============================================
+ Hits 27213 28698 +1485
+ Misses 9865 9432 -433
+ Partials 1799 1732 -67
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
1.8.0 only ships darwin x86_64 and needs Rosetta. 1.7.0 ships native arm64 under the misnamed darwin_x64 path. Intel Mac is not a supported local lint:ios host.
176071b to
93d2d11
Compare
russellwheatley
left a comment
russellwheatley
left a comment
There was a problem hiding this comment.
Java/ObjC facades look good from what I checked. One blocker (Android doesn't compile against SDK 37) plus a few things inline. I've left the dynamic RNFBFirebase probe out of this review, will pick that up separately.
| val task = taskInfo[0].taskInfo | ||
| currentActivity = | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) { | ||
| if (task.baseActivity != null) platform(task.baseActivity).shortClassName else "" | ||
| } else { | ||
| if (task.origActivity != null) { | ||
| platform(task.origActivity).shortClassName | ||
| } else { | ||
| platform(task.baseIntent.component).shortClassName |
There was a problem hiding this comment.
This is what's failing the Android e2e build. getTaskInfo() is annotated nullable in SDK 37 (what tests/android compiles against), so reading task.baseActivity / task.origActivity / task.baseIntent directly won't compile. I think the unit tests only pass because the package itself builds against compileSdk 34, but any app on a newer compile SDK would hit this too.
Old Java would have NPE'd here anyway, so maybe just skip it when null:
val task = taskInfo[0].taskInfo
if (task != null) {
currentActivity = ...
}| import java.util.ArrayList | ||
|
|
||
| class ReactNativeFirebaseJSON private constructor() { | ||
| private var jsonObject: JSONObject? = JSONObject(BuildConfig.FIREBASE_JSON_RAW) |
There was a problem hiding this comment.
The Java constructor wrapped this in a try/catch for JSONException and left jsonObject null. Here a parse failure would blow up the singleton init, and since TaskExecutorService calls getSharedInstance() in its init, every module would fail to construct after that. Gradle validates the JSON so it's unlikely, but might be worth keeping the try/catch for parity.
| static func create(googleAppID: String?, gcmSenderID: String?) -> NSObject { | ||
| #if canImport(RNFBFirebase) | ||
| RNFBFirebaseOptionsClient.create(googleAppID: googleAppID, gcmSenderID: gcmSenderID) | ||
| #else | ||
| // Pre-port forwarded nil into `initWithGoogleAppID:GCMSenderID:`. The Swift | ||
| // `FirebaseOptions(googleAppID:gcmSenderID:)` overlay requires non-optional String, | ||
| // so call the ObjC initializer directly to preserve nil. | ||
| let allocated = FirebaseOptions.perform(NSSelectorFromString("alloc"))! | ||
| .takeRetainedValue() as! NSObject | ||
| // `perform` returns +0. The extra retain is what `takeRetainedValue()` consumes | ||
| // when `init` returns the same instance, so the object is not over-released and | ||
| // still deallocated with its last owner. | ||
| _ = Unmanaged.passRetained(allocated) | ||
| return allocated.perform( | ||
| NSSelectorFromString("initWithGoogleAppID:GCMSenderID:"), | ||
| with: googleAppID, | ||
| with: gcmSenderID | ||
| )!.takeRetainedValue() as! NSObject | ||
| #endif |
There was a problem hiding this comment.
I think the retain counts here do balance, but it took me a while to convince myself, and the comments disagree with each other (the doc comment says both calls are +1, the inline one says perform returns +0).
If the only reason is passing nil through to the ObjC initializer, could we just do FirebaseOptions(googleAppID: googleAppID ?? "", gcmSenderID: gcmSenderID ?? "")? As far as I can tell JS always sends appId and messagingSenderId, so the nil case might not matter in practice.
| #endif | ||
| #import <React/RCTBridgeModule.h> | ||
|
|
||
| @class FIRApp; |
There was a problem hiding this comment.
Dropping the FirebaseCore import here (and in RCTConvert+FIRApp.h / RCTConvert+FIROptions.h, plus Photos in RNFBUtilsModule.h) is fine for everything in this repo. I checked and all the packages import FirebaseCore / Photos themselves. Third-party code that includes these headers and relied on the transitive import would stop compiling though. Probably worth a changelog note so it's not a surprise.
93d2d11 to
c4f4a79
Compare
A Debug e2e link needs CDP symbols that the Release Hermes engine does not export. Select the engine from the Xcode configuration.
Behavior change vs the pre-port Objective-C: the old code cast the option value to a pointer, so any non-nil value (including JS false) enabled automatic data collection. It now uses [dataCollection boolValue], so NO, 0 and NSNull disable it. This is intentional. Also adds host-side unit tests for the App and Utils TurboModule shells and restructures the @catch handling in RNFBAppModuleImplementation.
The import guard now scans .h/.hpp/.m/.mm/.c/.cpp, unprefixed quoted headers and digit separators. FIROptions allocation no longer uses perform(alloc)/passRetained, force unwraps are gone from RNFBAppModuleFirebase, and unused RNFBFirebase API is removed except RNFBFirebaseAppClient.configure(). Test coverage for the touched Swift, bridge and Ruby code is extended.
Debug cocoapods jobs could restore a Pods directory saved by the Release archive job, which had the Release Hermes engine swapped in, and failed linking facebook::hermes::cdp symbols. The cache key and restore key now include the build mode.
A failing xcodebuild -showBuildSettings was swallowed and reported as a missing app. Capture the exit status, print the stderr tail and fail with the real cause.
The iOS unit test host stub RNFBAppTurboModules.h sits next to the codegen header under packages/app/ios, so findSingleMatch found two matches and the Jest parity suite failed in CI. Skip HostStubs directories and cover the lookup with a unit test.
The dynamic Firebase probe pins firebase-ios-sdk with exact: in packages/app/ios/RNFBFirebase/Package.swift, and SDK bumps only touch sdkVersions.ios.firebase in packages/app/package.json. Add a Node script that fails with both values on mismatch and run it as the first step of the probe job.
The probe only checked that RNFBApp.framework does not define FIRApp, which does not show FirebaseCore is linked once. In probe mode the build now also requires RNFBFirebase.framework to define FIRApp, no standalone FirebaseCore framework in the products, and no other framework binary or the app executable defining it. The checks live in a sourced helper with Jest tests against stubbed and real nm output.
The probe job only built, so it never proved RNFBFirebase.framework is embedded and loads, or that RNFBFirebaseAppClient.configure() runs. The launch smoke now builds into its own DerivedData and, in probe mode, checks the facade symbol, the embedded and mapped framework, and the FirebaseCore configure log. It also reaps the log stream on failure.
…plicit Swift condition canImport(RNFBFirebase) picked the probe path in shipped code based on whether a module with that name happened to be importable. Probe mode now sets RNFB_DYNAMIC_FIREBASE_PROBE on the RNFBApp pod and the app targets, and a flag-off install removes it again, so only the probe compiles that path.
This branch has not been deployed
2 participants
Ports the
@react-native-firebase/appnative helpers to Kotlin (Android) and Swift (iOS) behind unchanged public Java/ObjC facades: handle map, preferences, JSON/meta, shared utils, event emitter, RCTConvert, Utils and the App module. Perf, Auth and Remote Config slices are on follow-up PRs.Also in here:
clang-formatfor Apple SiliconFirebaseCorecopy check, launch smoke)Possible breaking change (iOS)
initializeAppnow readsautomaticDataCollectionEnabledas a boolean. Before, it was a pointer cast(BOOL)[appConfig valueForKey:...], so any non-nil value, includingfalse, turned data collection on. Nowfalse,0,nulland absent turn it off.This fixes the incorrect behaviour, but anyone passing
falseand (unknowingly) getting collection anyway would see it change on upgrade. Commit is3c8714cebif we'd rather pull it into its ownfixPR. @mikehardy what do you think, keep here or split out?Smaller one:
RNFBSharedUtils.h,RCTConvert+FIRApp.handRCTConvert+FIROptions.hno longer importFirebaseCore(andRNFBUtilsModule.hno longer imports Photos). Every package in this repo imports what it needs, but third-party code relying on the transitive import would need to add it.