-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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] ConfigRealtime #14299
[rc-swift] ConfigRealtime #14299
Conversation
Generated by 🚫 Danger |
Apple API Diff ReportCommit: 13d4c70 FirebaseRemoteConfigClasses[MODIFIED] FIRRemoteConfigSettings[MODIFIED] FIRRemoteConfigSettingsSwift:
+ class RemoteConfigSettings : NSObject
- var minimumFetchInterval : TimeInterval { get set }
- var fetchTimeout : TimeInterval { get set }
Objective-C:
+ @interface FIRRemoteConfigSettings : NSObject
- @property ( nonatomic ) NSTimeInterval minimumFetchInterval ;
- @property ( nonatomic ) NSTimeInterval fetchTimeout ; FIRRemoteConfig[REMOVED] -initWithAppName:FIROptions:namespace:DBManager:configContent:userDefaults:analytics:configFetch:Swift:
- init ( appName : String , firOptions options : FIROptions , namespace FIRNamespace : String , dbManager DBManager : RCNConfigDBManager , configContent : RCNConfigContent , userDefaults : UserDefaults ?, analytics : ( any FIRAnalyticsInterop )?, configFetch : RCNConfigFetch ?)
Objective-C:
- - ( instancetype ) initWithAppName :( NSString * ) appName FIROptions :( FIROptions * ) options namespace :( NSString * ) FIRNamespace DBManager :( RCNConfigDBManager * ) DBManager configContent :( RCNConfigContent * ) configContent userDefaults :( nullable NSUserDefaults * ) userDefaults analytics :( nullable id < FIRAnalyticsInterop > ) analytics configFetch :( nullable RCNConfigFetch * ) configFetch ; [MODIFIED] -addOnConfigUpdateListener:Swift:
+ func addOnConfigUpdateListener ( remoteConfigUpdateCompletion listener : @escaping ( FIRRemoteConfigUpdate ?, ( any Error )?) -> Void ) -> FIRConfigUpdateListenerRegistration
- func addOnConfigUpdateListener ( remoteConfigUpdateCompletion listener : @escaping ( FIRRemoteConfigUpdate ?, ( any Error )?) -> Void ) -> ConfigUpdateListenerRegistration
Objective-C:
+ - ( FIRConfigUpdateListenerRegistration * _Nonnull ) addOnConfigUpdateListener : ( FIRRemoteConfigUpdateCompletion _Nonnull ) listener ;
- - ( FIRConfigUpdateListenerRegistration * _Nonnull ) addOnConfigUpdateListener : ( FIRRemoteConfigUpdateCompletion _Nonnull ) listener ; [ADDED] configRealtimeSwift:
+ var configRealtime : RCNConfigRealtime { get set }
Objective-C:
+ @property ( nonatomic , readwrite , strong , nonnull ) RCNConfigRealtime * configRealtime [ADDED] -initWithAppName:FIROptions:namespace:DBManager:configContent:userDefaults:analytics:configFetch:configRealtime:Swift:
+ init ( appName : String , firOptions options : FIROptions , namespace FIRNamespace : String , dbManager DBManager : RCNConfigDBManager , configContent : RCNConfigContent , userDefaults : UserDefaults ?, analytics : ( any FIRAnalyticsInterop )?, configFetch : RCNConfigFetch ?, configRealtime : RCNConfigRealtime ?)
Objective-C:
+ - ( instancetype ) initWithAppName :( NSString * ) appName FIROptions :( FIROptions * ) options namespace :( NSString * ) FIRNamespace DBManager :( RCNConfigDBManager * ) DBManager configContent :( RCNConfigContent * ) configContent userDefaults :( nullable NSUserDefaults * ) userDefaults analytics :( nullable id < FIRAnalyticsInterop > ) analytics configFetch :( nullable RCNConfigFetch * ) configFetch configRealtime :( nullable RCNConfigRealtime * ) configRealtime ; [REMOVED] FIRConfigUpdateListenerRegistration[REMOVED] FIRConfigUpdateListenerRegistrationSwift:
- class ConfigUpdateListenerRegistration : NSObject , @unchecked Sendable
- func remove ()
Objective-C:
- @interface FIRConfigUpdateListenerRegistration : NSObject
- - ( void ) remove ; |
@@ -26,15 +26,11 @@ import Foundation | |||
// TODO(ncooke3): Once Obj-C tests are ported, all `public` access modifers can be removed. | |||
|
|||
#if RCN_STAGING_SERVER | |||
private let serverURLDomain = "https://staging-firebaseremoteconfig.sandbox.googleapis.com" | |||
private let serverURLDomain = "staging-firebaseremoteconfig.sandbox.googleapis.com" |
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.
Now using URLComponents to construct the URL
#endif | ||
private let serverURLVersion = "/v1" |
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.
Now in UtilsURL.swift
@@ -741,29 +728,6 @@ extension Installations: InstallationsProtocol {} | |||
dataTask.resume() | |||
} | |||
|
|||
private func constructServerURL() -> String { |
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.
To UtilsURL
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.
Leaving a first pass review
self.propagateErrors(error) | ||
return | ||
} | ||
if self.canMakeConnection() { |
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.
optional, but consider moving this up to the guard condition:
guard self.canMakeConnection(), self.remainingRetryCount > 0 else { ... }
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.
Not sure about that, since it could introduce more errors when there are no listeners?
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.
Thanks for detailed review
self.propagateErrors(error) | ||
return | ||
} | ||
if self.canMakeConnection() { |
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.
Not sure about that, since it could introduce more errors when there are no listeners?
Co-authored-by: Nick Cooke <[email protected]>
Merging here. @ncooke3 will address remaining comments in a subsequent PR. |
Co-authored-by: Nick Cooke <[email protected]>
This is an initial Swift implementation of ConfigRealtime.
Some of the implementation was borrowed from similar implementations in ConfigFetch and a Utils directory was started to avoid copying shared functions.
Most tests are disabled and on deeper analysis far from sufficient. They should still be implemented in Swift once we have everything in Swift. However, integration testing will be necessary since most of the tricky code is related to URLSession management for which the unit tests are especially lacking.