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

Re-land "Delete engine v1 android embedding" #52022

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

gmackall
Copy link
Member

@gmackall gmackall commented Apr 10, 2024

Re-lands #51229

The only change is to un-delete the PluginRegistry.Registrar interface from the v1 PluginRegistry. The reason is that we recommended plugin authors to include a static method referencing this type in our own docs on v1->v2 migration, so deleting it would be a breaking change for lots of existing plugins.*

Other failures from the initial attempt are fixed in flutter/flutter#146523

*There is more discussion at flutter/packages#6494

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@gmackall gmackall marked this pull request as ready for review April 10, 2024 20:36
@gmackall gmackall requested a review from a team April 10, 2024 21:04
@gmackall
Copy link
Member Author

cc @stuartmorgan, as we talked about this on flutter/packages#6494

@gmackall
Copy link
Member Author

Hmm actually I need to retain the contents of the interface as well, part of which references the deprecated FlutterView as well...

converting to draft to figure this out

@gmackall gmackall marked this pull request as draft April 10, 2024 22:52
@gmackall
Copy link
Member Author

gmackall commented Apr 11, 2024

I think this unfortunately is actually going to have to wait until we warn plugin authors, which will happen in the next stable release.

io.flutter.plugin.common.PluginRegistry.Registrar references the deprecated FlutterView, which in turn references much of the rest of the v1 embedding. And there are plugins depending on being able to get a FlutterView from their PluginRegistry.Registrar in their registerWith method, because we told them to.

I've requested to include a section in the next "whats new in" announcement, warning plugin authors to remove this method. As soon as the next stable is released, and that announcement is made, I'll continue forward with this deletion.

@johnmccutchan
Copy link
Contributor

I think this unfortunately is actually going to have to wait until we warn plugin authors, which will happen in the next stable release.

io.flutter.plugin.common.PluginRegistry.Registrar references the deprecated FlutterView, which in turn references much of the rest of the v1 embedding. And there are plugins depending on being able to get a FlutterView from their PluginRegistry.Registrar in their registerWith method, because we told them to.

I've requested to include a section in the next "whats new in" announcement, warning plugin authors to remove this method. As soon as the next stable is released, and that announcement is made, I'll continue forward with this deletion.

Questions I have:

  • Is the code that the plugins are using marked deprecated?
  • If so, when did we mark it deprecated?
  • How long have we shipped an alternative API that plugins could have ported to?
  • Which plugins specifically are impacted?

@gmackall
Copy link
Member Author

  • Is the code that the plugins are using marked deprecated?

My understanding is that the code the plugins are using isn't even part of an interface, so we don't have an effective method of deprecating it. We simply told them to include a method (which references a deprecated type from the v1 embedding), and then v1 apps would call that method expecting it existed? I'm not 100% sure on that though (how it gets called - I am 100% sure it isn't in the FlutterPlugin interface)

  • How long have we shipped an alternative API that plugins could have ported to?

There isn't a direct alternative. One method was for supporting v1 apps, and the other was for supporting v2 apps. At the time of the v1 embedding deprecation our recommendation was to support both. We haven't given further guidance since then, to my knowledge.

  • Which plugins specifically are impacted?

16 of our own here flutter/packages#6494. Also the integration_test plugin that lives in the framework repo was, until flutter/flutter#146523. I don't know about the rest of the ecosystem, but I could check.

@gmackall
Copy link
Member Author

Also because https://docs.flutter.dev/release/breaking-changes/plugin-api-migration was written for an audience of plugin developers familiar with the v1 embedding, it might not be clear what I mean when I say "we told them to" reference a deprecated type - its specifically this portion

Also, note that the plugin should still contain the static registerWith() method to remain compatible with apps that don't use the v2 Android embedding. (See Upgrading pre 1.12 Android projects for details.) The easiest thing to do (if possible) is move the logic from registerWith() into a private method that both registerWith() and onAttachedToEngine() can call. Either registerWith() or onAttachedToEngine() will be called, not both.

This registerWith method that they would have had existing in their plugins already, has a signature
public static void registerWith(@NonNull io.flutter.plugin.common.PluginRegistry.Registrar registrar)

@stuartmorgan
Copy link
Contributor

We haven't given further guidance since then, to my knowledge.

That's correct, because in previous discussions the agreement was that we would remove tool support for v1 apps (on stable) before we told plugin authors (including ourselves) to remove v1 support from their plugins. Because IMO it doesn't make sense for us to tell the ecosystem to not support a build configuration that we are still supporting in the tool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants