-
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
Add .flutter-plugins-dependencies
to the project, which contains the app's plugin dependency graph
#45379
Add .flutter-plugins-dependencies
to the project, which contains the app's plugin dependency graph
#45379
Conversation
@tvolkert This is the last thing we need to complete the plugin migration to the new embedding. |
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.
Implementation LGTM % tests
@@ -265,29 +292,52 @@ List<Plugin> findPlugins(FlutterProject project) { | |||
return plugins; | |||
} | |||
|
|||
/// Returns true if .flutter-plugins has changed, otherwise returns false. | |||
/// Returns true if .flutter-plugins or .flutter-plugins-dependencies have changed, |
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.
Ultra-nit, // instead of ///
It's not a full dart doc since it only partially describes this function (its return value). If ever this becomes a public method, it's up to that person to write a proper dart doc
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.
///
makes the comment show up in the IDE.
Can we put this somewhere other than the project root? |
@jonahwilliams this is intended to be used by all platforms. However, this PR only implements the functionality for Android. |
@jonahwilliams It represents a conceptual inter-dependency between plugins for all platforms. If we intend to have a different pub dependency per platform, then it might make sense to have multiple such files. |
9be0ea5
to
4ad2762
Compare
@blasten and I spoke offline. It may need to be moved when we do platform specific plugins but I'm okay with it being here for now |
f8e4bb2
to
44f9aee
Compare
.flutter-plugins-dependencies
to the project, which contains the app's plugin dependency graph
tests LGTM |
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 handle cyclic dependencies? For example, plugin a and b depend on each other.
@jonahwilliams Here's the output (when there's a cycle):
|
@@ -250,11 +275,13 @@ Plugin _pluginFromPubspec(String name, Uri packageRoot) { | |||
return null; | |||
} | |||
final String packageRootPath = fs.path.fromUri(packageRoot); | |||
final YamlMap dependencies = pubspec['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.
This will capture direct dependencies, but not transitive dependencies - is this an issue?. Consider:
plugin A depends on package 1
package 1 depends on plugin B
.flutter-plugins will contain plugin A/plugin B while the dependencies file will not link them.
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.
From offline discussion, this isn't a supported use case - and that platform code cannot depend on eachother. Therefore it doesn't seem like the gradle task ordering needs to take this into consideration
Co-Authored-By: Jonah Williams <jonahwilliams@google.com>
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
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.
Is there an integration test for this using a plugin that needs this feature?
f585bae
to
ccd5d68
Compare
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 w/ question
@zanderso definitely. Once, the Google maps plugin is published, I can add an integration test. I will file an issue. |
FYI - I won't be publishing the migration because I was just attempting to prove that Lifecycle works with plugins. So we'll be waiting on the ecosystem team to do the public migration. |
Description
Creates a
.flutter-plugins-dependencies
which contains the plugin dependency graph from pub. Then, Gradle reads this file to reconstruct the dependencies.Related Issues
Fixes #45188
Tests
I added the following tests: unit test
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
). This will ensure a smooth and quick review process.///
).flutter analyze --flutter-repo
) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?