fix: sync init flow changed to support dynamic update #297
Conversation
jaeopt
commented
Feb 14, 2020
jaeopt
commented
- Add an option to dynamically update ProjectConfig on datafile cache update for sync init flow (this feature is disabled by default to be compatible with the old versions).
- When periodic polling is enabled, the sync cache change always update ProjectConfig dynamically regardless of the setting above.
| /// - doFetchDatafileBackground: This is for debugging purposes when | ||
| /// you don't want to download the datafile. In practice, you should allow the | ||
| /// background thread to update the cache copy (optional) | ||
| public func start(datafile: Data, doFetchDatafileBackground: Bool = true) throws { | ||
| public func start(datafile: Data, | ||
| doUpdateConfigOnNewDatafile: Bool = false, |
There was a problem hiding this comment.
These two parameters don't really make sense here. If you have doUpdateConfigOnNewDatafile as true but you have doFeatchDatafileBackground as false. It is an invalid combination. Maybe an enum might be better here with all valid combinations. Also, the default combination is probably not ideal if you have polling enabled which is not addressed in this combo or PR. So, you might want combos that include polling and what to do there.
There was a problem hiding this comment.
Yes it's not so clean as it should be. The reason I keep "doFetchDatafileBackground" is for API backward compatibility only, where we had this option for debugging support. We can let them override "doUpdateConfigOnNewDatafile" when they still want to disable fetch completely for debugging purpose.
| @@ -39,6 +40,14 @@ open class OptimizelyClient: NSObject { | |||
| } | |||
|
|
|||
| let eventLock = DispatchQueue(label: "com.optimizely.client") | |||
|
|
|||
| private var isPeriodicPollingEnabled: Bool { | |||
There was a problem hiding this comment.
nit: can we make this an extension?
