Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Feature Flag Tools π¨ #705
Feature Flag Tools π¨ #705
Changes from 10 commits
41f3f2c
8827eff
5edd5f4
3606748
c075528
63c6e0a
f005e41
8b424f6
6d370f3
30197b3
1bf874e
e39736b
9631a01
a40f9ea
1691523
160fae5
e4c07a2
0d9f895
f150e83
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is there a reason why we are updating the environment on the view controller? Here we could pass the environment using and input like
didUpdateEnvironment(environment)
and let the ViewModel handle the updating action, this way we take this responsibility out of the view controller.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.
Just one thing to bear in mind when doing that sort of thing inside the view model is - if the environment is involved in the chain of signals that perform that replacement you may run the risk of the environment being switched out from underneath and the subscriptions being lost. Little tricky to explain but I had this happen once before something like:
Because this depends on the current environment doing the replacement here can be problematic. This might not exactly apply in what @Scollaco is suggesting π My feeling is to do the replacement outside of the view model as it is now.
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.
Yeah I thought about this and my feeling is that actually updating the environment is more of a side effect. This way, we can be certain that the right
EnvironmentType
is being sent, and we've separately tested that side-effect behavior works as expected.We also do it this way in
AppDelegate