feature branch: refactor experimentation to not use FluxC#244
feature branch: refactor experimentation to not use FluxC#244
experimentation to not use FluxC#244Conversation
To make it more readable
We don't need milliseconds precision here, so it's better to have the same unit for `timeToLive` and `fetchedAt`
`experimentStore` is no longer used
This makes maintenance of the SDK easier, and it's not a big setback for clients, as the value for `platform` has to be only checked once.
Even though the ExPlat doesn't work correctly (we don't ever save data in cache) the `ExPlatTest` suite is passing, just because everything here is mocked.
Also, makes the ignored test pass
Majority of experiments I've been testing require `anonId` parameter, hence this change. Allowing consumers to specify `anonId` is also something that iOS counterpart does: https://github.com/Automattic/Automattic-Tracks-iOS/blob/ccf6b7d7424e32857819d35aa54813875557f5c8/Sources/Experiments/ExPlat.swift#L65 I've decided to introduce another state field in `ExPlat` for convenience of library consumers. They'll likely inject `ExPlat` class as Singleton, so clearing/reconfiguring its state on e.g. user change will be easier than injecting another ExPlat to DI graph. iOS counterpart uses Singleton pattern.
…non_id` There's no value in sending an empty parameter
So SDK consumers can build `ExPlat` instance
In order to remove `runBlocking` from `ExPlat`, we have to have a way to get value from cache in a way, that doesn't require sync file-operation. That's why I introduce `latest` field, which keeps the latest cache value in memory. Providing dispatchers is considered a good practice and might come handy during tests
Now, that we have few new/changed tests, I extract some similarities to make tests more readable
…instead of mock invocation
…nts, even if cache is fresh`
This is a confusing API, because, even if user requests a refresh, we don't return fresh data immediately, but only queue this to fetch. This is not something I'd expect as the SDK consumer. Also, we already have specialized refresh methods. I don't think we need more. I plan to remove `shouldRefreshIfStale` in another commit.
This commit adds a new class: `CacheDto` that saves previously saved data with addition to `anonymousId`. This additional field will be used for cache validation.
The `initialize` method now takes a mandatory `anonymousId` parameter and is responsible for setting the anonymous identifier and triggering the initial assignment refresh and cache validation if `anonymousId` is different
I came to a conclusion, that `refresh` is not needed for the client: SDK should on its own manage the state of the cache and not delegate additional responsibility to SDK consumer. The cache is refreshed whenever needed during initialization
As SDK doesn't offer refreshing anymore, we don't need to use internal-only `RefreshStrategy`. Refactor seems successful as unit tests are still passing without any changes
If ExPlat was not initialized, log or throw an exception for quicker debugging
It doesn't have to be exposed to SDK consumers
This can be used by WP/JP or WC apps
Maybe it can be useful to, e.g., be able to add network interceptors for logging or performance monitoring
…mandatory Make anon id mandatory. Simplify API, remove `refresh`
Experiments public contract
Add auth token
Reviewhi @necronet 👋 . This PR consist several smaller PRs and some of the code is Android platform-specific, so I thought about leaving some context to make the review easier.
|
necronet
left a comment
necronet
left a comment
There was a problem hiding this comment.
LGTM I checked through the code and the test app. Making the right call and with the right parameters.
As requested the fluxc and wellsql dependencies have been removed, and these changes now use a filebased system for caching the variations in the Repository class provided to ExPlat.
I'd enforce a non-empty anonymous id throught this sdk though instead of anonymousId?.let { addQueryParameter("anon_id", it) } but I am not sure if it will break something else, as this something that we discussed it should be fine, API can handle this scenario quite well.
|
Thank you for the review @necronet !
That's a good point, but this is how the proposed change works now: When The |
After reading the comment again, I've enhanced this guard with a |
2 participants
Previously merged:
ExperimentRestClientand basic domain models #237Contains:
anon_idto ExPlat #240refresh#245