add optional features Data to initializer with API connection#98
add optional features Data to initializer with API connection#98vazarkevych wants to merge 21 commits intogrowthbook:mainfrom
Conversation
madhuchavva
left a comment
There was a problem hiding this comment.
It needs to more changes to support default / fallback features incase of internet connection failures.
To add more context, if we just modify the initializer to accept fallbackFeatures along with the API connection, we'll need graceful mechanisms to use these fallbackFeatures and retry the API calls if backgroundSync is turned on.
Nevertheless, Every time we use features to evaluate, first, try regular cache (previously fetched from API) and in case of cache-miss or invalidated scenario, we use these fallback features for evaluation.
We'll need to log these events carefully, otherwise debugging any unexpected behaviors/results will be very tricky.
|
Hey @madhuchavva, just to clarify - are you proposing that we don’t persist the passed features to the cache, and instead keep them in a separate field, and use them only if cache is empty or invalid? |
|
Hi @madhuchavva, here’s what we’ve done regarding fallback features and connection handling:
Could you please review and let us know if this logic looks good to you or if you'd suggest any improvements? |
# Conflicts: # Sources/CommonMain/Features/FeaturesViewModel.swift # Sources/CommonMain/GrowthBookSDK.swift
# Conflicts: # Sources/CommonMain/Features/FeaturesViewModel.swift # Sources/CommonMain/GrowthBookSDK.swift
# Conflicts: # GrowthBook-IOS.xcodeproj/project.pbxproj # GrowthBookTests/ExperimentRunTests.swift # GrowthBookTests/FeaturesViewModelTests.swift # GrowthBookTests/GrowthBookSDKBuilderTests.swift # GrowthBookTests/Source/json.json # Sources/CommonMain/Caching/CachingManager.swift # Sources/CommonMain/Evaluators/ConditionEvaluator.swift # Sources/CommonMain/Evaluators/ExperimentEvaluator.swift # Sources/CommonMain/Evaluators/FeatureEvaluator.swift # Sources/CommonMain/Features/FeaturesViewModel.swift # Sources/CommonMain/GrowthBookSDK.swift # Sources/CommonMain/Model/Context.swift # Sources/CommonMain/Model/Feature.swift # Sources/CommonMain/Model/GlobalContext.swift # Sources/CommonMain/Model/StickyAssignmentsDocument.swift # Sources/CommonMain/StickyBucket/StickyBucketService.swift # Sources/CommonMain/Utils/Constants.swift # Sources/CommonMain/Utils/Utils.swift # Conflicts: # GrowthBookTests/FeaturesViewModelTests.swift # GrowthBookTests/GrowthBookSDKBuilderTests.swift # Sources/CommonMain/Caching/CachingManager.swift # Sources/CommonMain/Evaluators/ExperimentEvaluator.swift # Sources/CommonMain/Evaluators/FeatureEvaluator.swift # Sources/CommonMain/Features/FeaturesViewModel.swift # Sources/CommonMain/GrowthBookSDK.swift # Sources/CommonMain/Model/Feature.swift # Sources/CommonMain/Model/StickyAssignmentsDocument.swift # Sources/CommonMain/StickyBucket/StickyBucketService.swift # Sources/CommonMain/Utils/Constants.swift # Sources/CommonMain/Utils/Utils.swift
# Conflicts: # GrowthBookTests/FeaturesViewModelTests.swift # GrowthBookTests/GrowthBookSDKBuilderTests.swift # Sources/CommonMain/Features/FeaturesViewModel.swift # Sources/CommonMain/GrowthBookSDK.swift # Sources/CommonMain/Model/Context.swift # Sources/CommonMain/Utils/Constants.swift # Sources/CommonMain/Utils/Utils.swift
madhuchavva
left a comment
There was a problem hiding this comment.
I reviewed these changes thoroughly but couldn't run the tests though! Here are my findings.
-
[P1] Fallback can override a valid cached feature set (wrong precedence).
InfetchFeatures, cached features are loaded first, but if the network call fails and fallbackFeatures exists, fallback is applied unconditionally, replacing cache-derived features. This conflicts with the intended order “cache first, fallback only on cache miss/invalid”.
See the lines : FeaturesViewModel.swift#L101, FeaturesViewModel.swift#L120, FeaturesViewModel.swift#L130 -
[P1] @objc initializer API is source-breaking for Objective-C consumers.
AddingfallbackFeaturesin the middle of the existing@objcinitializer changes the generated Objective-C selector. Existing ObjC call sites using the old selector will fail to compile. This should be introduced via an overload (or appended parameter strategy), not by changing the existing selector shape.
See the lines : GrowthBookSDK.swift#L51, GrowthBookSDK.swift#L56 -
[P2] Fallback payload parsing is too narrow and silently drops valid API-shaped data.
featuresinitialization supports bothFeaturesDataModeland rawFeatures, butfallbackFeaturesonly decodes rawFeatures. If callers pass API-style JSON, fallback becomes nil with no warning, making failures hard to diagnose.
References: GrowthBookSDK.swift#L257, GrowthBookSDK.swift#L291, GrowthBookSDK.swift#L293 -
Possibly a missing behavior gap - [P2] Remote-eval failure path ignores fallback/retry semantics.
InremoteEvalmode, failures only emit.failedToLoadData; there is no fallback usage and no retry/backoff behavior tied tobackgroundSync. This leaves the “graceful offline/default behavior” incomplete for one of the main fetch paths.
References: FeaturesViewModel.swift#L103, FeaturesViewModel.swift#L109
I believe the points 1 and 2 are regressions. Happy to discuss more if you have any questions.
# Conflicts: # Sources/CommonMain/Caching/CachingManager.swift # Sources/CommonMain/GrowthBookSDK.swift # Sources/CommonMain/Utils/Utils.swift
Hi @madhuchavva
|
No description provided.