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

tests #12133

Closed
wants to merge 2 commits into from
Closed

tests #12133

wants to merge 2 commits into from

Conversation

themiswang
Copy link
Contributor

draft pr

@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

Copy link
Contributor

github-actions bot commented Nov 22, 2023

Apple API Diff Report

Commit: 0653198
Last updated: Thu Nov 23 11:30 PST 2023
View workflow logs & download artifacts


FirebaseCrashlytics

Classes

[MODIFIED] FIRCrashlytics
[MODIFIED] FIRCrashlytics
Swift:
+  class Crashlytics : NSObject
-  class Crashlytics : NSObject
-    class func crashlytics () -> Self
-    func log ( _ msg : String )
-    func log ( format : String , arguments args : CVaListPointer )
-    func setCustomValue ( _ value : Any ?, forKey key : String )
-    func setCustomKeysAndValues ( _ keysAndValues : [ AnyHashable : Any ])
-    func setUserID ( _ userID : String ?)
-    func record ( error : Error )
-    func record ( error : Error , userInfo : [ String : Any ]? = nil )
-    func record ( exceptionModel : ExceptionModel )
-    func didCrashDuringPreviousExecution () -> Bool
-    func setCrashlyticsCollectionEnabled ( _ enabled : Bool )
-    func isCrashlyticsCollectionEnabled () -> Bool
-    func checkForUnsentReports () async -> Bool
-    func checkAndUpdateUnsentReports () async -> CrashlyticsReport ?
-    func sendUnsentReports ()
-    func deleteUnsentReports ()
Objective-C:
+  @interface FIRCrashlytics : NSObject < FIRRolloutsStateSubscriber >
-  @interface FIRCrashlytics : NSObject
-    + ( nonnull instancetype ) crashlytics ;
-    - ( void ) log :( nonnull NSString * ) msg ;
-    - ( void ) logWithFormat :( nonnull NSString * ) format , ...;
-    - ( void ) logWithFormat :( nonnull NSString * ) format arguments :( struct __va_list_tag * ) args ;
-    - ( void ) setCustomValue :( nullable id ) value forKey :( nonnull NSString * ) key ;
-    - ( void ) setCustomKeysAndValues :( nonnull NSDictionary * ) keysAndValues ;
-    - ( void ) setUserID :( nullable NSString * ) userID ;
-    - ( void ) recordError :( nonnull NSError * ) error ;
-    - ( void ) recordError :( nonnull NSError * ) error userInfo :( nullable NSDictionary < NSString * , id > * ) userInfo ;
-    - ( void ) recordExceptionModel :( nonnull FIRExceptionModel * ) exceptionModel ;
-    - ( BOOL ) didCrashDuringPreviousExecution ;
-    - ( void ) setCrashlyticsCollectionEnabled :( BOOL ) enabled ;
-    - ( BOOL ) isCrashlyticsCollectionEnabled ;
-    - ( void ) checkForUnsentReportsWithCompletion :( nonnull void ( ^ )( BOOL )) completion ;
-    - ( void ) checkAndUpdateUnsentReportsWithCompletion : ( nonnull void ( ^ )( FIRCrashlyticsReport * _Nullable )) completion ;
-    - ( void ) sendUnsentReports ;
-    - ( void ) deleteUnsentReports ;

FirebaseRemoteConfig

Classes

FIRRemoteConfig
[ADDED] -addRemoteConfigInteropSubscriber:
Objective-C:
+  - ( void ) addRemoteConfigInteropSubscriber :( nonnull id < FIRRolloutsStateSubscriber > ) subscriber ;
FIRRemoteConfigValue
[ADDED] subscribers
Swift:
+  var subscribers : NSMutableArray { get }
Objective-C:
+  @property ( nonatomic , readonly , nonnull ) NSMutableArray < id < FIRRolloutsStateSubscriber >> * subscribers

@@ -23,6 +23,17 @@
#import "Interop/Analytics/Public/FIRAnalyticsInterop.h"

@implementation FIRRemoteConfigComponent
static FIRRemoteConfigComponent *_sharedDefaultAppRemoteConfigSingleton = nil;

+ (FIRRemoteConfigComponent *)sharedDefaultAppRemoteConfigSingletonWithDefaultApp:(FIRApp *)app {
Copy link
Contributor Author

@themiswang themiswang Nov 23, 2023

Choose a reason for hiding this comment

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

We probably need some strategy to create a singleton for the default app Provider and Interop. They will return the same object.

Copy link
Contributor Author

@themiswang themiswang Nov 23, 2023

Choose a reason for hiding this comment

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

Checked RC Component from Crashlytics and FirePerf, confirm they have the same RC Component: https://screenshot.googleplex.com/9NXgjPbmtxoSrSe

@@ -16,7 +16,7 @@

#import "FIRCrashlyticsReport.h"
#import "FIRExceptionModel.h"

@protocol FIRRolloutsStateSubscriber;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we import this?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this can be imported.

Here's an example where AppCheck imports the AppCheckInterop header in order to declare conformance with the FIRAppCheckProtocol protocol from the interop SDK.

Copy link
Member

@ncooke3 ncooke3 Dec 4, 2023

Choose a reason for hiding this comment

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

I think it needs to be imported per https://stackoverflow.com/a/11533804/9331576. Were you able to get things building in its current state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting, I see in codebase that we have some pattern for forward declaring a protocol, for example FireAuth: https://github.com/firebase/firebase-ios-sdk/blob/master/FirebaseAuth/Sources/Backend/FIRAuthRequestConfiguration.h#L24. I am able to get things build for now. But I'll change to import if it's safer.

Copy link
Contributor

@samedson samedson left a comment

Choose a reason for hiding this comment

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

Looks great. Love the singleton technique.

Copy link
Member

@ncooke3 ncooke3 left a comment

Choose a reason for hiding this comment

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

Adding some review suggestions for missing file copyrights. I also added some feedback in the DD with regard to some of the API names.

Comment on lines +1 to +6
//
// File.swift
//
//
// Created by Themis Wang on 2023-11-16.
//
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//
// File.swift
//
//
// Created by Themis Wang on 2023-11-16.
//
// Copyright 2023 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

Comment on lines +1 to +6
//
// File.swift
//
//
// Created by Themis Wang on 2023-11-16.
//
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//
// File.swift
//
//
// Created by Themis Wang on 2023-11-16.
//
// Copyright 2023 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

Comment on lines +1 to +6
//
// File.swift
//
//
// Created by Themis Wang on 2023-11-16.
//
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
//
// File.swift
//
//
// Created by Themis Wang on 2023-11-16.
//
// Copyright 2023 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

@themiswang
Copy link
Contributor Author

Adding some review suggestions for missing file copyrights. I also added some feedback in the DD with regard to some of the API names.

@ncooke3 Thank you for the review!

@themiswang
Copy link
Contributor Author

Finished prototype review, gonna close this pr.

@themiswang themiswang closed this Dec 5, 2023
@firebase firebase locked and limited conversation to collaborators Jan 5, 2024
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.

5 participants