Make feature flags more readable/supportable. #4828
Conversation
| /// ] | ||
| const bool _kEnableExperiments = bool.fromEnvironment('enable_experiments'); | ||
|
|
||
| @visibleForTesting |
There was a problem hiding this comment.
since this isn't hidden is visibleForTesting necessary here?
There was a problem hiding this comment.
visibleForTesting protects public members from use in non-test environment: https://api.flutter.dev/flutter/meta/visibleForTesting-constant.html
It cannot make private methods public.
polina-c
changed the title
| const bool _kEnableExperiments = bool.fromEnvironment('enable_experiments'); | ||
|
|
||
| @visibleForTesting | ||
| bool enableExperiments = _kEnableExperiments; |
There was a problem hiding this comment.
why can't we just do this:
final bool enableExperiments = _kEnableExperiments || !kReleaseMode;
Then we don't need a method setExperimentsEnabled at all
There was a problem hiding this comment.
It is for protection.
enableExperiments is private and protected from switching back and force.
It can be flipped only once to true.
There was a problem hiding this comment.
we don't need it to be switched at all though. We don't need to enable experiments from main.dart, we can just set the value from the start in feature_flags.dart. The following code would still allow us to disable experiments for tests if that is a concern:
@visibleForTesting
bool enableExperiments = _kEnableExperiments || !kReleaseMode;
There was a problem hiding this comment.
How can we disable experiments for all tests? As I know there is no method like 'setupForAllTestLibraries'. And we do not want to update every single test, right?
There was a problem hiding this comment.
It's a tradeoff we have to consider here. I'm not a huge fan of making our production code messier for the sake of tests, but the tradeoff would be to manually disable experiments for tests that shouldn't have them on, which doesn't scale well. I also don't know of a flutter test configuration where we could set a value like this for all tests. Let's leave it as you have it.
There was a problem hiding this comment.
I believe all tests should test production setup by default, otherwise we will have reliability issues. So, disabling tests manually sounds to be weak choice.
But we can chose between two options:
- Have production code messier by turning off experiments (this PR)
- Have experiments off in debug mode, with necessity to pass flags if we want to see them while we develop the app
There was a problem hiding this comment.
After discussing this a little bit since we made the original change, I think option 2 is our best step forward. In an offline discussion with @jacob314, he also voiced concerns that having experiments on by default could prevent us from catching issues that only show up when experiments are disabled, which is what our users will see.
3 participants
No description provided.