-
Notifications
You must be signed in to change notification settings - Fork 6k
Migrate embedding to AndroidX #17075
Conversation
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.
Do we have a good handle on the compatibility issues this could introduce for consumers? It seems like it should be better now than a year ago but I've lost touch with some of this.
As a nit you can ignore if you want: it might be better to move the download logic to CIPD rather than relying on maven. Maven has been pretty stable for us with this though so ... it's probably fine either way. The advantage of CIPD would be that we could more easily express this in our DEPS file and gclient would know how to track it properly.
@dnfield This shouldn't introduce any issue since we deprecated non-androidx builds in the previous stable release. I regard to versions, we aren't setting any version constraints at this point since Flutter projects aren't using Gradle 6.0 yet.
Sounds like we should have a bug to track that. However, we still need to generate pom files out of files.json, so we would need to keep some of that logic in the engine repo. |
fyi @xster |
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.
LGTM
Should we still send out a "breaking change" announcement? I think it was still previously possible to create a host app in Support and use the engine in Support in 1.12 right? Now using jetifier would be necessary in the host app? |
* | ||
* <p>// Create a ShimPluginRegistry and wrap the FlutterEngine with the shim. ShimPluginRegistry | ||
* shimPluginRegistry = new ShimPluginRegistry(flutterEngine, platformViewsController); | ||
* <pre> |
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.
:)
The actual implementation LGTM. Thanks for finding all the matches. |
@xster I thought we did that already last quarter. Otherwise, we should send the announcement now that flutter/flutter#52340 is checked in. |
c11582d
to
74cb082
Compare
What about existing projects? If I had an add-to-app project created with 1.12 and both are on support and I flutter upgrade after this, I'd have to jetify my host app then right? (I think that's probably ok, but we should have some announcement/wiki) |
7f57940
to
63a9bbf
Compare
Thats actually not really okay. Project using add-to-app which are currently unable to update to androidX will be broken. As of 1.17 today they actually are. |
@tal-mi would jetifying your project be possible? |
If I create a library that depends on support libraries and a flutter module then I would have to stay on 1.12. I may have clients which will not want to update their apps to AndroidX so the best way for a "flutter build arr" flow is to provide an (androidX true/false) that will link the embedded libs accordingly. |
Thanks for the context. I would suggest 2 approaches: 1- Create 2 libraries. 1 on support with a 1.12 Flutter module and 1 on AndroidX with a 1.17 module.
It's not quite just toggling upstream dependencies. The Flutter engine itself consumes support/AndroidX so it would need different engines altogether. And one instance of the framework isn't tested to work with multiple variants of the engine. |
Thanks for the advice. |
This reverts commit 025e2d8
This reverts commit 025e2d8
Fixes flutter/flutter#39283
Merge this PR after flutter/flutter#52340 has been merged.