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

Decide how to handle Dart plugin registration for direct implementation dependencies #99083

Closed
stuartmorgan opened this issue Feb 24, 2022 · 3 comments · Fixed by #122046
Closed
Assignees
Labels
a: plugins Support for writing, building, and running plugin packages P2 Important issues not at the top of the work list tool Affects the "flutter" command-line tool. See also t: labels.

Comments

@stuartmorgan
Copy link
Contributor

Dart plugin registration uses the implementing-package resolution system described in the federated plugin design doc to figure out which plugins to run Dart registration for. At a high level the rules are:

  • If there's a direct dependency from the app on an implementation package, use that (to allow overriding a default implementation).
  • Otherwise, use the default implementation.

This works well for normal federated plugin usage. We recently ran into an edge case, which is that sometimes a package might depend only on a specific platform implementation. For example: shared_preference_windows depends directly on path_provider_windows (but not path_provider), because we need the functionality to implement shared_preferences_windows, but we wouldn't want to cause, say, the Android and iOS apps of anything using shared_preferences to unnecessarily include the path_provider code for those platforms. In this case:

  • the application has no dependency on the implementation package, and
  • there is no default implementation (because the concept of "default implementation" is defined by the app-facing package that we aren't using)
    so Dart registration doesn't run for path_provider_windows. This is fine if we use PathProviderWindows directly (which we do), but breaks if we try to use PathProviderPlatform.instance (which is the equivalent of what the PR above did) for reasons that are probably not at all clear to anyone hit by it.

We have a few options here:

  1. Keep the current behavior of not running registration
  2. Run registration for implementation packages whose app-facing packages are not in the transitive dependency list. The question then becomes what to do in the edge case where two different implementations are in the transitive dependency list:
    • Pick one of them arbitrarily.
    • Error out and require the application that is ultimately the transitive consumer of the two to pick one to depend on.

I'm leaning toward thinking we should do some form of 2 since I would expect the case where two different packages pick two different implementation packages for the same platform to be very rare, so 2 should give surprising behavior less often.

@stuartmorgan stuartmorgan added tool Affects the "flutter" command-line tool. See also t: labels. plugin P2 Important issues not at the top of the work list labels Feb 24, 2022
@Xiphoseer
Copy link

I just ran into a variant of this, which I feel is slightly more confusing than the rules set out above. Consider the following packages:

  • my_app (application)
  • my_auth (pure dart library)
  • flutter_appauth (pub.dev implementation plugin)
  • flutter_appauth_platfrom_interface (pub.dev platform interface package)
  • flutter_appauth_desktop (my extension to add/prototype windows and linux support)

The idea would be to have all appauth plugins be an implementation detail of my_auth and for my_app to depend on that library. To that end, I'd like to have a way to make sure that the non-endorsed flutter_appauth_desktop plugin is registered. The current language on docs.flutter.dev on that is (emphasis mine):

A developer can still use your implementation, but must manually add the plugin to the app’s pubspec file. So, the developer must include both the foobar dependency and the foobar_windows dependency in order to achieve full functionality.

https://docs.flutter.dev/development/packages-and-plugins/developing-packages#non-endorsed-federated-plugin

Personally, I missed that this really only refers to direct dependencies of a top-level application and packages depending on (non-endorsed) plugin implementations are not supported. This could probably be emphasized more, but there's something else:

As this issue confirms, this only applies to the dart portion of plugins. The flutter_appauth_platform_interface package happens implement and use a platform implementation based on MethodChannel as the default instance. If I add a flutter.plugin.platforms.windows.pluginClass value to the pubspec, this is registered and usable.

Only when I try to use dartPluginClass is that not picked up, unless the plugin is added to the app pubspec. This difference in behavior seems confusing and unintentional, and it's unfortunate because I do have a pure dart implementation for the package and don't really want to drop down to C++ if I can avoid it.

So implementing 2. as proposed would make things even more confusing for non-endorsed plugins: If I transitively depend only on flutter_appauth_desktop, the dartPluginClass should start working on those platforms as long as I don't also depend on flutter_appauth.

Some actionable ideas, that might help directing developers in the right direction:

  • With the current behavior: Warn (or error) if a plugin sets flutter.plugin.implements, but is not endorsed and is not a direct dependency of the application. Right now, these are partially registered as independent native plugins.
  • Warn if the app-facing package in implements is not in the dependency tree at all. Currently, I can set implements: foobar in pubspec and nothing complains.
  • If the intent is not to register non-endorsed plugins, consider restricting the ability of non-implements native plugins to addMessageListener on channels created by other plugins. This seems to be difficult to implement in a backwards-compatible way, because the current APIs to create a MessageChannel don't necessarily require using a Registrar or other means of linking the channel registration to the current plugin.
  • If that is not the intent, consider lifting the non-endorsement restriction and register all dartPluginClasses in the dependency tree unconditionally, as is done for native pluginClass and only error/pick one when there are multiple colliding implementations in the dependency tree.

@stuartmorgan
Copy link
Contributor Author

This difference in behavior seems confusing and unintentional

It is unintentional in that we haven't yet extended the resolution system back to the existing native registration system. That is the intent, at which point the behaviors will be consistent.

  • If that is not the intent, consider lifting the non-endorsement restriction and register all dartPluginClasses in the dependency tree unconditionally, as is done for native pluginClass and only error/pick one when there are multiple colliding implementations in the dependency tree.

This seems reasonable; I agree that the use case you described should work as you expected that it would, and I don't see any issues with enabling that approach.

I suspect that the focus on direct dependencies (other than for conflict resolution) in the original design document was just an oversight.

@stuartmorgan stuartmorgan self-assigned this Jul 8, 2022
@stuartmorgan stuartmorgan added the a: plugins Support for writing, building, and running plugin packages label Feb 9, 2023
stuartmorgan added a commit to stuartmorgan/flutter that referenced this issue Mar 6, 2023
Fixes issues related to transitive dependencies on plugins with Dart
implementations:
- Ensures that if there are multiple transitive dependencies including
  the app-facing-package-selected default, the default is picked.
  Previously it was order-dependent what would happen, and in some
  cases no implementation would be picked.
- Allows registration of a package's implementation even if the
  app-facing package is not present, as long as there is only one
  possible implementation (or the app directly picks one). There are
  cases where depending on just a platform implementation of a package,
  rather than the app-facing package, is useful. (E.g.,
  shared_preferences_linux depends directly on path_provider_linux.)

Also fixes a bug where an implementation conflict error was being gated
behind a flag that was only supposed to be hiding an (overly-strict at
the moment) error about plugin pubspec definitions.

Fixes flutter#99083
Fixes flutter#118401
@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a: plugins Support for writing, building, and running plugin packages P2 Important issues not at the top of the work list tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants