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

[rc-swift] RemoteConfigValue #14245

Merged
merged 6 commits into from
Dec 12, 2024
Merged

[rc-swift] RemoteConfigValue #14245

merged 6 commits into from
Dec 12, 2024

Conversation

paulb777
Copy link
Member

@paulb777 paulb777 commented Dec 11, 2024

Perhaps the most notable thing about this transition is needing to make a few items public for the FirebasePerformance internal dependency since we can no longer rely upon internal Objective-C headers.

#no-changelog

Copy link
Contributor

github-actions bot commented Dec 11, 2024

Apple API Diff Report

Commit: 8c951b5
Last updated: Thu Dec 12 10:54 PST 2024
View workflow logs & download artifacts


FirebaseRemoteConfig

Classes

[REMOVED] FIRRemoteConfigValue
[REMOVED] FIRRemoteConfigValue
Swift:
-    var stringValue : String { get }
-    var numberValue : NSNumber { get }
-    var dataValue : Data { get }
-    var boolValue : Bool { get }
-    var jsonValue : Any ? { get }
-    var source : RemoteConfigSource { get }
-    init ( data : Data ?, source : RemoteConfigSource )
Objective-C:
-    @property ( nonatomic , readonly , nonnull ) NSString * stringValue ;
-    @property ( nonatomic , readonly , nonnull ) NSNumber * numberValue ;
-    @property ( nonatomic , readonly , nonnull ) NSData * dataValue ;
-    @property ( nonatomic , readonly ) BOOL boolValue ;
-    @property ( nonatomic , readonly , nullable ) id JSONValue ;
-    @property ( nonatomic , readonly ) FIRRemoteConfigSource source ;
-    - ( instancetype _Nonnull ) initWithData :( nullable NSData * ) data source :( FIRRemoteConfigSource ) source ;
FIRRemoteConfig
[MODIFIED] -defaultValueForKey:
Swift:
+  func defaultValue ( forKey key : String ?) -> FIRRemoteConfigValue ?
-  func defaultValue ( forKey key : String ?) -> RemoteConfigValue ?
Objective-C:
+  - ( nullable FIRRemoteConfigValue * ) defaultValueForKey :( nullable NSString * ) key ;
-  - ( nullable FIRRemoteConfigValue * ) defaultValueForKey :( nullable NSString * ) key ;
[MODIFIED] -configValueForKey:
Swift:
+  func configValue ( forKey key : String ?) -> FIRRemoteConfigValue
-  func configValue ( forKey key : String ?) -> RemoteConfigValue
Objective-C:
+  - ( nonnull FIRRemoteConfigValue * ) configValueForKey :( nullable NSString * ) key ;
-  - ( nonnull FIRRemoteConfigValue * ) configValueForKey :( nullable NSString * ) key ;
[MODIFIED] -configValueForKey:source:
Swift:
+  func configValue ( forKey key : String ?, source : RemoteConfigSource ) -> FIRRemoteConfigValue
-  func configValue ( forKey key : String ?, source : RemoteConfigSource ) -> RemoteConfigValue
Objective-C:
+  - ( nonnull FIRRemoteConfigValue * ) configValueForKey :( nullable NSString * ) key source : ( FIRRemoteConfigSource ) source ;
-  - ( nonnull FIRRemoteConfigValue * ) configValueForKey :( nullable NSString * ) key source : ( FIRRemoteConfigSource ) source ;
[MODIFIED] -objectForKeyedSubscript:
Swift:
+  subscript ( key : String ) -> FIRRemoteConfigValue { get }
-  subscript ( key : String ) -> RemoteConfigValue { get }
Objective-C:
+  - ( nonnull FIRRemoteConfigValue * ) objectForKeyedSubscript : ( nonnull NSString * ) key ;
-  - ( nonnull FIRRemoteConfigValue * ) objectForKeyedSubscript : ( nonnull NSString * ) key ;

@paulb777
Copy link
Member Author

Test failures are flakes/expected. pod-gen unit tests are running locally

@paulb777 paulb777 requested a review from ncooke3 December 12, 2024 00:14
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.

Nothing important to address, LGTM!

Comment on lines +18 to +19

@import FirebaseRemoteConfig;
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
@import FirebaseRemoteConfig;

FirebaseRemoteConfig/SwiftNew/RemoteConfigValue.swift Outdated Show resolved Hide resolved
FirebaseRemoteConfig/SwiftNew/RemoteConfigValue.swift Outdated Show resolved Hide resolved
Copy link
Member Author

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

Thanks for the thoughtful review!

Comment on lines 54 to 56
guard !dataValue.isEmpty else {
return nil
}
Copy link
Member

@ncooke3 ncooke3 Dec 12, 2024

Choose a reason for hiding this comment

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

There is still one small behavior difference. If a non-nil, empty data object was used in ObjC, it would make it past the nil check and try to serialize. I just tested what happens when you serialize an empty data object and you get an error (Error Domain=NSCocoaErrorDomain Code=3840 "Unable to parse empty data." UserInfo={NSDebugDescription=Unable to parse empty data.}).

So the net difference is that non-nil empty data would exit with nil here, while ObjC implementation would throw and catch the serialization error, log the message, and return nil. Removing line 54-56 would match ObjC implementation. The main difference is that the early exit here misses the log message.

@paulb777 paulb777 merged commit f922738 into rc-swift Dec 12, 2024
41 of 46 checks passed
@paulb777 paulb777 deleted the pb-rcvalue branch December 12, 2024 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants