Integrate Firestore with Firebase platform logging #6507
Conversation
Generated by 🚫 Danger |
Firestore/core/src/util/config.h
Outdated
| @@ -1,57 +0,0 @@ | |||
| /* | |||
There was a problem hiding this comment.
This file shouldn't be deleted (unless there's a replacement?).
Note that config.h was previously generated, but this was changed for compatibility with Swift Package Manager which does not allow for build-time configuration scripts.
There was a problem hiding this comment.
Sorry, originally I deleted the wrong config.h file. Thanks for explaining!
| @@ -0,0 +1,33 @@ | |||
| /* | |||
There was a problem hiding this comment.
This was the old location of the file (before we shortened all the paths). This should be removed.
|
|
||
| class FirebasePlatformLoggingApple : public FirebasePlatformLogging { | ||
| public: | ||
| explicit FirebasePlatformLoggingApple(FIRApp* app); |
There was a problem hiding this comment.
Objective-C types should have their nullability defined, either by wrapping in NS_ASSUME_NONNNULL_BEGIN/END or by annotating the pointers directly (FIRApp* _Nonnull app) .
| #import "FirebaseCore/Sources/Private/FIRAppInternal.h" | ||
| #import "FirebaseCore/Sources/Private/FIRHeartbeatInfo.h" | ||
| #import "FirebaseCore/Sources/Private/FIROptionsInternal.h" | ||
| //#import "FirebaseCore/Sources/Public/FirebaseCore/FIRApp.h" |
There was a problem hiding this comment.
Please remove commented-out code.
| * Wraps the platform-dependent functionality associated with Firebase platform | ||
| * logging. | ||
| */ | ||
| class FirebasePlatformLogging { |
There was a problem hiding this comment.
In the Android implementation, we have a notion of a GrpcMetadataProvider which does everything required to set the headers. Doing something similar here could make this a much narrower interface.
On the other hand, an interface like this one makes this information independent of the gRPC implementation which has some merit. Do you have any thoughts on how to make these platforms work similarly? This doesn't seem like something that has to be different on the different platforms.
There was a problem hiding this comment.
When I started working on this, I indeed wanted to avoid a dependency on gRPC. However, this resulted in an interface that requires a lot of handholding from the client code (it must make sure to call all the IsFooAvailable functions, for example), so it seems that a dependency is a small price to pay for a more self-sufficient class.
Mimicking Android would be a bigger change -- the Android interface is pretty abstract, and it seems like it should provide all the extra metadata (including Cloud headers). If you feel strongly about this, I can do this in a follow-up.
There was a problem hiding this comment.
The Android implementation is also imperfect. My original thought there was that separate concerns could supply separate metadata providers. That is, Firebase headers from one provider, Cloud headers from another, Auth perhaps from yet another. From that point of view, I think what you have here fits that goal quite well. Whether or not you see any benefit is another matter.
There was a problem hiding this comment.
My original thought there was that separate concerns could supply separate metadata providers.
That's pretty much how I see it as well.
The sample user agent string I'm getting in XCode is
apple-platform/ios apple-sdk/17E8258 fire-fst/1.17.1 fire-ios/6.10.2 swift/true xcode/11E503a.