engine icon indicating copy to clipboard operation
engine copied to clipboard

Delete v1 android engine embedding

Open gmackall opened this issue 1 year ago • 8 comments

Fixes https://github.com/flutter/flutter/issues/143531

Other failures from the initial attempt are fixed in https://github.com/flutter/flutter/pull/146523/

Pre-launch Checklist

  • [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • [x] I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • [x] I listed at least one issue that this PR fixes in the description above.
  • [x] 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.
  • [x] I updated/added relevant documentation (doc comments with ///).
  • [x] I signed the [CLA].
  • [x] All existing and new tests are passing.

gmackall avatar Apr 10 '24 16:04 gmackall

cc @stuartmorgan, as we talked about this on https://github.com/flutter/packages/pull/6494

gmackall avatar Apr 10 '24 21:04 gmackall

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 avatar Apr 10 '24 22:04 gmackall

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.

gmackall avatar Apr 11 '24 19:04 gmackall

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?

johnmccutchan avatar Apr 11 '24 23:04 johnmccutchan

  • 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 https://github.com/flutter/packages/pull/6494. Also the integration_test plugin that lives in the framework repo was, until https://github.com/flutter/flutter/pull/146523. I don't know about the rest of the ecosystem, but I could check.

gmackall avatar Apr 11 '24 23:04 gmackall

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)

gmackall avatar Apr 11 '24 23:04 gmackall

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.

stuartmorgan-g avatar Apr 12 '24 13:04 stuartmorgan-g

This is blocked on plugin outreach and soak time.

gmackall avatar Jun 07 '24 18:06 gmackall

Planning to land this this afternoon 🙏

gmackall avatar Oct 23 '24 18:10 gmackall