Skip to content
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

Merged
merged 50 commits into from
Apr 23, 2021

Conversation

blasten
Copy link

@blasten blasten commented Apr 2, 2021

Relands #74469 and incorporates #77959 from @cyanglaz + additional changes in 829756e, 28f74e4.

The last two commits handle this situation:

  1. It doesn't generate generated_main.dart if there isn't need for it. That is, no plugin uses dartPluginClass in the pubspec.

  2. 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.

  3. Use the new target in all cases.

Fixes #52267

@flutter-dashboard flutter-dashboard bot added d: examples Sample code and demos team Infra upgrades, team productivity, code health, technical debt. See also team: labels. tool Affects the "flutter" command-line tool. See also t: labels. labels Apr 2, 2021
@google-cla google-cla bot added the cla: yes label Apr 2, 2021
@blasten blasten marked this pull request as ready for review April 3, 2021 00:44
/// 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?;
Copy link
Member

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?

Copy link
Author

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.

Copy link
Member

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);
  });

packages/flutter_tools/lib/src/flutter_plugins.dart Outdated Show resolved Hide resolved
packages/flutter_tools/lib/src/flutter_plugins.dart Outdated Show resolved Hide resolved
expect(const DartPluginRegistrantTarget().canSkip(environment2), false);
});

testUsingContext("doesn't generate generated_main.dart if there aren't Dart plugins", () async {
Copy link
Member

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.

Copy link
Author

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/lib/src/flutter_plugins.dart Outdated Show resolved Hide resolved
packages/flutter_tools/lib/src/flutter_plugins.dart Outdated Show resolved Hide resolved
@zanderso
Copy link
Member

@jonahwilliams ptal.

Copy link
Member

@jonahwilliams jonahwilliams left a 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?;
Copy link
Member

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);
  });

@tvolkert
Copy link
Contributor

This has been verified to have caused #91841

auto-submit bot pushed a commit that referenced this pull request Mar 7, 2024
…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".
auto-submit bot pushed a commit that referenced this pull request Jul 11, 2024
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
d: examples Sample code and demos team Infra upgrades, team productivity, code health, technical debt. See also team: labels. tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement dartPluginClass support for plugins
5 participants