-
Notifications
You must be signed in to change notification settings - Fork 21
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
Feat/bootstrapping #96
Conversation
Note: If poller injected then bootstrap from client init ingnored
Includes adding stub to test target refactor: Removes unused test util
Sources/UnleashProxyClientSwift/Poller/DictionaryStorageProvider.swift
Outdated
Show resolved
Hide resolved
@@ -155,9 +149,12 @@ public class UnleashClientBase { | |||
@available(iOS 13, tvOS 13, *) | |||
public class UnleashClient: UnleashClientBase, ObservableObject { | |||
@MainActor | |||
public func start(_ printToConsole: Bool = false) async throws { | |||
public func start( | |||
bootstrap: Bootstrap = .toggles([]), |
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.
with bootstrap going as a first parameter isn't it gonna be a breaking API change?
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 don't think it should since it has a default argument. However, if printToConsole
is used, it now needs to be explicit since without, at the call-site it would be like this:
// Without explicit printToConsole argument, confusing what the "true" applies to at a glance
await start(bootstrap: .toggles([...]), true)`
// With explicit printToConsole argument, clearer what the bool input applies to
await start(bootstrap: .toggles([...]), printToConsole: true)
// Default arguments so following should work
await start() // default: no bootstrap toggles, and printToConsole false
await start(bootstrap: .toggles([...])) // boostrapped, default: printToConsole false
await start(printToConsole: true) // default: no bootstrap toggles, override: printToConsole
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.
fair point
case .toggles(let toggles): | ||
return toggles | ||
|
||
case .jsonFile(let path): |
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 wondering if we could make it more similar to the android SDK where we pass a pre-existing file instead of a path to a file. Would it eliminate the fata error where file cannot be found and move it to the user space? From a quick check android SDK checks if the provided file exists and uses default when it doesn't.
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 did want to do this but Swift doesn't really have a concept of a File object. However for the fatal error we could instead default to an empty list of toggles (a default). I wasn't sure whether to do that or not as since it may be misleading to a user that they input a file path and seemingly no issues reading from it even if invalid. Hence alerting the user earlier seemed more appropriate telling them the file they expected to exist is not actually on their filesystem. What do you think?
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.
maybe printing warning to the console that the file was not found and using a default empty list?
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.
Sure! I'll change that over and put some doc comment on it which should hopefully surface 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.
Awesome work. The change looks really solid. I add some minor comments inline. I'm curious what you think about them.
Also one question comment about the docs? Would you prefer to handle a docs change in this PR or a separate one?
Thank you for the review :) I responded to the comments and would like to know what you think. Also I'm happy to update README here, just wanted to confirm approach was appropriate first :) |
Your design decisions seem to be spot on :) I'm willing to approve it and merge asap so that others can benefit from your work. |
Awesome! I just pushed up change to use default empty list of toggles if json file cannot be found, and also updated the readme :) I re-requested review just for a double check |
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.
We're good to go live with it
About the changes
This PR adds bootstrapping of toggles and has taken inspiration from both the js and Android client SDKs.
I also took the opportunity to tidy up the folder structure so the files are more focused are structured more where expected.
For the bootstrapping I added it to both initialisation of UnleashClient & Poller, and also when calling
start
on the client.Bootstrapping is passed into each of above via an enum specifying whether it is from a list of
Toggle
's, or from a json file hosted with the host application. The.jsonFile(path)
is meant to be explicit enough so that it is up to the host application to correctly specify file location. Further, if file cannot be found I settled on a fatalError since if the file doesn't exist in the project it should inform the developer.For passing in toggles at client initialisation, due to existing way client is initialised (e.g. passing in all parameters for the Poller, and also the poller), if bootstrap and a poller is injected, the bootstrap injected into the client will be ignored and must be directly included into Poller instead. I considered updating this initialiser so that there aren't redundant injected properties in case of Poller added, but that felt like a breaking change better suited for a major release.
Closes #92
Important files
Unfortunately due to way files are tracked when moving, the PR gives impression more files changed than actually are since I moved into more logically arrangements that makes it significantly easier to read.
Discussion points
If this approach is suitable, I will add a commit to update the readme on usage as well, just wanted to confirm approach first.