-
Notifications
You must be signed in to change notification settings - Fork 27.1k
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
Reland the Dart plugin registry #79669
Reland the Dart plugin registry #79669
Conversation
…r#74469" (flutter#78623)" This reverts commit 5efc716.
5fc5024
to
ea3ffe1
Compare
/// Contains the name of the dependencies. | ||
/// These are the keys specified in the `dependency` map. | ||
Set<String> get dependencies { | ||
final YamlMap? dependencies = _descriptor['dependencies'] as YamlMap?; |
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.
Does this imply that there should be more validation of this section in the _validate
method below? Or would we not even get this far if the dependencies
section is broken?
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.
Yup. The YAML parser would fail prior to reaching this section. Also,dependencies
should always be present since the flutter sdk section is always present in dependencies
.
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.
Dependencies is not required to be present, but the code you added does cover if it is missing. Consider adding a unit test for this:
testWithoutContext('FlutterManifest can parse empty dependencies', () async {
const String manifest = '''
name: test
''';
final BufferLogger logger = BufferLogger.test();
final FlutterManifest? flutterManifest = FlutterManifest.createFromString(
manifest,
logger: logger,
);
expect(flutterManifest, isNotNull);
expect(flutterManifest!.dependencies, isEmpty);
});
expect(const DartPluginRegistrantTarget().canSkip(environment2), false); | ||
}); | ||
|
||
testUsingContext("doesn't generate generated_main.dart if there aren't Dart plugins", () 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.
Here and below, just asking to confirm that these tests can't be testWithoutContext
.
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.
There are a few global dependencies that touches other functions as well. I managed to get these three tests to use testWithoutContext
, but in doing so, I broke almost all the plugins test.
packages/flutter_tools/test/general.shard/dart_plugin_test.dart
Outdated
Show resolved
Hide resolved
de725d3
to
7f110f3
Compare
7f110f3
to
22f32ed
Compare
e8db2c5
to
1124bbc
Compare
1124bbc
to
31ac2dc
Compare
@jonahwilliams ptal. |
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 with a nit
/// Contains the name of the dependencies. | ||
/// These are the keys specified in the `dependency` map. | ||
Set<String> get dependencies { | ||
final YamlMap? dependencies = _descriptor['dependencies'] as YamlMap?; |
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.
Dependencies is not required to be present, but the code you added does cover if it is missing. Consider adding a unit test for this:
testWithoutContext('FlutterManifest can parse empty dependencies', () async {
const String manifest = '''
name: test
''';
final BufferLogger logger = BufferLogger.test();
final FlutterManifest? flutterManifest = FlutterManifest.createFromString(
manifest,
logger: logger,
);
expect(flutterManifest, isNotNull);
expect(flutterManifest!.dependencies, isEmpty);
});
This has been verified to have caused #91841 |
…on (#144214) Part of #137040 and #80374 The flag `throwOnPluginPubspecError` never actually was enabled during production in #79669, but only in some dart plugin tests. And in the tests the case of the error when enabling the flag was not explicitly tested. The only thing tested was, that it is not thrown when disabled. As explained [here](#142035 (comment)) the only case, where this error could be thrown is, when a dart implementation and a native inline implementation are provided simultaneously. But throwing an exception there is a wrong behavior, as both can coexist in a plugin package, thus in the pubspec file. Disabling the flag means, that the error is not thrown and not shown to the user. This is the case in production (contrary to the dart plugin tests), which acts like these plugin cases of implementations are just skipped. And this is what actually should be done. In conclusion, I think the case of coexisting dart and native implementation in pubspec was just overlooked and therefore this error validation was introduced, which is not necessary or even valid. For more discussion, see: https://discord.com/channels/608014603317936148/608022056616853515/1200194937791205436 - This is tricky: I already added a test in #142035, which finally complies with the other tests, by removing the flag. So I think it falls in the category of "remove dead code". - Theoretically this is a breaking change, as removing / altering some tests. But the flag actually was never valid or used, so IDK. But this may not does fall in the category of "contributed tests".
These changes allow to override existing native endorsed (federated & inline) plugins with e.g. non-endorsed plugin implementations via direct dependencies as described in the section *Platform implementation selection* in the [design doc](https://docs.google.com/document/d/1LD7QjmzJZLCopUrFAAE98wOUQpjmguyGTN2wd_89Srs). ```yaml # pubspec.yaml name: example dependencies: my_plugin: ^0.0.1 my_plugin_android_alternative: ^0.0.1 ``` Closes #80374, closes #59657 Related: Override Dart plugins (#87991, #79669)
Relands #74469 and incorporates #77959 from @cyanglaz additional changes in 829756e, 28f74e4.
The last two commits handle this situation:
It doesn't generate
generated_main.dart
if there isn't need for it. That is, no plugin usesdartPluginClass
in the pubspec.generated_main.dart
is deleted if there isn't any platform resolution. This is the case if you delete a plugin from pubspec.yaml and then hot reload/restart.Use the new target in all cases.
Fixes #52267