-
Notifications
You must be signed in to change notification settings - Fork 13
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
Defaults and cache #33
Conversation
@wicheda do we know why the ubuntu-build workflow is failing here? |
@matthewelwell , quite strange it failed, it must be a slightly different build version on Ubuntu. However, it's also lucky since it was actually calling the wrong method on UserDefaults, I've fixed it now. |
completion(result) | ||
switch result { | ||
case .success(let flags): | ||
self.updateCache(flags: flags, forIdentity: identity) |
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.
It's a little unclear to me here why we're still passing a value for forIdentity
here. If I'm understanding the logic correctly, identity
is always nil at this point?
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.
True, it is always nil in the else block, but really we're just saying pass whatever identity was passed into this method, whether it is nil or not. I think that's safer since if for example someone refactored it and moved around the else block, they may miss the identity part and not pass it through.
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.
I guess this is why it would be nice to have unit test coverage here, but that's another conversation!
For readability's sake, I do think that I prefer to be explicit here, so I think we should either omit the forIdentity
parameter (as I think it defaults to nil
) or pass nil
explicitly here. That being said, I'm not sure that my feelings on this one are strong enough to argue if you are keen to leave it as it is.
let value = flags.first(where: {$0.feature.name == id})?.value | ||
completion(.success(value?.stringValue)) | ||
var flag = flags.first(where: {$0.feature.name == id}) | ||
flag = self.getFlagUsingCacheAndDefaults(withID: id, flag: flag, forIdentity: identity) |
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.
I'm not too sure why we're redefining the value of flag
again here?
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.
Ah, I think I understand now that I've read the getFlagUserCacheAndDefaults
method. As I understand it, if the flag is not nil
, then this method just returns the flag that was passed in. This does seem a little bit convoluted though. Wouldn't it be easier to understand if we just did something like:
var flag = flags.first(where: {$0.feature.name == id})
if flag === nil {
flag = self.getFlagUsingCacheAndDefaults(withID: id, forIdentity: identity)
}
maybe it's a few more lines of code, but I think it's a lot more readable? Maybe I'm also missing something though.
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.
Your interpretation is correct, I just thought it would be better for brevity's sake, but can change it.
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.
I think let's go with readability, yes please.
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.
OK, this has been done.
} | ||
|
||
/// Return an array of flag for an identity, including the cached flags (if enabled) and the default flags when they are not already present in the passed array | ||
private func getFlagsUsingCacheAndDefaults(flags:[Flag], forIdentity identity: String? = nil) -> [Flag] { |
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.
Again, forgive me if I'm mistaken here but I believe this could cause some slightly unusual behaviour if a flag is removed from the API for example, but not yet from the cache or defaults. Users will continue to get a value for the old flag, but fresh values for the other features. I think we should only rely on the cache / defaults based on connectivity issues or general caching rules.
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.
That's a business decision I guess, but happy to take a steer from you and @dabeeeenster . So the question is, when a user retrieves all flags, should we always add the cached and default flags to the returned list, or only do that in the case of a failed call?
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.
In the case of defaults, yes, that's for sure how it should work. In the case of cached flags, I would expect there to be some level of TTL on the local cache which would negate the need for the API request in the first place. As per the options in the Javscript client documentation here.
In either case, I would not expect a mix of 'fresh' flags and cached or default flags.
…e case of failures, not successful calls.
Hi @dabeeeenster / @matthewelwell , I'm back from my trip and have found some time to finish this work. So, I have:
I've also made the stylistic changes you requested @matthewelwell . I could look at adding tests but noticed there aren't currently any tests at the Flagsmith level yet, so it may be a fairly lengthy task to create those since it will be quite tricky to start making real calls, and simulating failed connections etc. I'd have to schedule that over the next couple of weeks if you did want it, so let me know. I also edited the documents repository and made a PR there. |
|
||
// set cache on / off (defaults to on) |
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.
I'm not sure that we should default it to on, unless we want to major version this? It will logically change the behaviour of the SDK for people upgrading, right (even if that change is for the better)?
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.
Yes, fair point.
private let NIL_IDENTITY_KEY = "nil-identity" | ||
|
||
/// Cached flags to fall back on if an API call fails, by identity | ||
private var cachedFlags: [String:[Flag]] = [:] |
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.
@wicheda I'm concerned that this is not the correct pattern we should be following here for caching. Is there not a standard interface that we can use for iOS caching? It's important to us (based on a recent customer's requirements) that they can use their own caching implementation to add additional encryption for example. I'd expect to be able to override the implementation of some standard caching interface and provide this as part of the client instantiation.
Let me know if this doesn't make sense - happy to jump on a call to discuss further if needs be.
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.
@matthewelwell , if we want to integrate the cacheing at a deeper (and modular) level, I'll have to look into that next week and see what is feasible with the existing HTTP architecture, but that may be doable.
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.
Yes, that would be great. Let me know if you want to chat. You can catch me on Slack as needed.
superseded by #34 |
Adding:
Can be merged alongside Flagsmith/flagsmith#2237