-
Notifications
You must be signed in to change notification settings - Fork 29.2k
Add flutter: config: {...}
section to pubspec.yaml
to influence FeatureFlags
#167953
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
Add flutter: config: {...}
section to pubspec.yaml
to influence FeatureFlags
#167953
Conversation
/// The value is resolved, if possible, in the following order, where if a | ||
/// step resolves to a boolean value, no further steps are attempted. | ||
/// | ||
/// ## Environment Variable |
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.
Consider making it clearer this is an ordered list:
/// The value is resolved, if possible, in the following order, where if a | |
/// step resolves to a boolean value, no further steps are attempted. | |
/// | |
/// ## Environment Variable | |
/// The value is resolved, if possible, in the following order, where if a | |
/// step resolves to a boolean value, no further steps are attempted: | |
/// | |
/// ## 1. Environment Variable |
/// The value is resolved, if possible, in the following order, where if a | ||
/// step resolves to a boolean value, no further steps are attempted. | ||
/// | ||
/// ## Environment Variable |
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 current implementation, the environment variable has lower priority than the global config. Could we keep that order and make the local project configuration the highest priority?
/// | ||
/// [globalConfig] reads values stored by the `flutter config` tool, which | ||
/// are normally in the user's `%HOME` directory (varies by system), while | ||
/// [projectManfiest] reads values from the _current_ Flutter project's |
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.
/// [projectManfiest] reads values from the _current_ Flutter project's | |
/// [projectManifest] reads values from the _current_ Flutter project's |
expect( | ||
flutterMacOSDesktopFeature.generateHelpMessage(), | ||
'Enable or disable support for desktop on macOS.', | ||
testWithoutContext('reads from configuration if avaialble', () { |
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.
testWithoutContext('reads from configuration if avaialble', () { | |
testWithoutContext('reads from configuration if available', () { |
@@ -254,6 +254,8 @@ void main() { | |||
containsIgnoringWhitespace('Analytics reporting is currently enabled'), | |||
); | |||
}, overrides: <Type, Generator>{Analytics: () => fakeAnalytics}); | |||
|
|||
testUsingContext('overriden by the content in pubspec.yaml', () async {}); |
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.
Looks like this can be deleted
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.
Done.
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.
Looks good overall, one open question on ordering
Thanks for the review! Will come back to this shortly, agree on changes. |
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.
LGTM once @loic-sharma's comments are addressed.
}); | ||
} | ||
|
||
const Feature _noConfigFeature = Feature(name: 'example'); |
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.
Ubernit: should these constants be top of file for readability?
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.
Done.
}); | ||
|
||
/// Flutter Web | ||
FileSystem createFsWithPubspec(String pubspec) { |
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.
Nit: Since we're only using this in one place, can we instead hardcode the contents of pubspec
here and not take a parameter? That way it's obvious what the contents of the pubspec are without scrolling to the bottom of the test.
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.
Done.
…nfluence `FeatureFlags` (flutter/flutter#167953)
…nfluence `FeatureFlags` (flutter/flutter#167953)
…nfluence `FeatureFlags` (flutter/flutter#167953)
} | ||
return isEnabled; | ||
|
||
// Otherwise, read it from environment variable > project manifest > global config |
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.
FYI this comment is stale
…nfluence `FeatureFlags` (flutter/flutter#167953)
…nfluence `FeatureFlags` (flutter/flutter#167953)
…nfluence `FeatureFlags` (flutter/flutter#167953)
…nfluence `FeatureFlags` (flutter/flutter#167953)
…nfluence `FeatureFlags` (flutter/flutter#167953)
…nfluence `FeatureFlags` (flutter/flutter#167953)
…nfluence `FeatureFlags` (flutter/flutter#167953)
…nfluence `FeatureFlags` (flutter/flutter#167953)
…nfluence `FeatureFlags` (flutter/flutter#167953)
…nfluence `FeatureFlags` (flutter/flutter#167953)
…nfluence `FeatureFlags` (flutter/flutter#167953)
…nfluence `FeatureFlags` (flutter/flutter#167953)
…nfluence `FeatureFlags` (flutter/flutter#167953)
…nfluence `FeatureFlags` (flutter/flutter#167953)
…nfluence `FeatureFlags` (flutter/flutter#167953)
…nfluence `FeatureFlags` (flutter/flutter#167953)
…nfluence `FeatureFlags` (flutter/flutter#167953)
…nfluence `FeatureFlags` (flutter/flutter#167953)
…nfluence `FeatureFlags` (flutter/flutter#167953)
…nfluence `FeatureFlags` (flutter/flutter#167953)
…nfluence `FeatureFlags` (flutter/flutter#167953)
…nfluence `FeatureFlags` (flutter/flutter#167953)
…nfluence `FeatureFlags` (flutter/flutter#167953)
…nfluence `FeatureFlags` (flutter/flutter#167953)
…nfluence `FeatureFlags` (flutter/flutter#167953)
added missing '/' https://github.comflutter/flutter/pull/167953 => flutter/flutter#167953
…nfluence `FeatureFlags` (flutter/flutter#167953)
…nfluence `FeatureFlags` (flutter/flutter#167953)
…FeatureFlags` (flutter#167953) Closes flutter#167667. In this PR: - `FlutterFeaturesConfig` reads from the global config, current pubspec.yaml, and environment - `FeatureFlags` now delegates "was this configured" to `FlutterFeaturesConfig` - `featureFlags` (`Context`) now is created with knowledge of the current `pubpec.yaml`, if any Once this lands and we play with it a bit (i.e. migrate `disable-swift-package-manager`), we can publicize it and update website documentation (it won't make it into this beta/stable release anyway). I did a significant rewrite of `feature_test`, as lots of what was being tested no longer made sense.
…nfluence `FeatureFlags` (flutter/flutter#167953)
…nfluence `FeatureFlags` (flutter/flutter#167953)
…nfluence `FeatureFlags` (flutter/flutter#167953)
Closes #167667.
In this PR:
FlutterFeaturesConfig
reads from the global config, current pubspec.yaml, and environmentFeatureFlags
now delegates "was this configured" toFlutterFeaturesConfig
featureFlags
(Context
) now is created with knowledge of the currentpubpec.yaml
, if anyOnce this lands and we play with it a bit (i.e. migrate
disable-swift-package-manager
), we can publicize it and update website documentation (it won't make it into this beta/stable release anyway).I did a significant rewrite of
feature_test
, as lots of what was being tested no longer made sense.