Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[WIP] Delete deprecated v1 code #4457

Closed
wants to merge 2 commits into from
Closed

Conversation

GaryQian
Copy link
Contributor

@GaryQian GaryQian commented Oct 28, 2021

We plan on deleting the v1 Android embedding, so we should no longer use the deprecated code.

Part of flutter/flutter#92643 which is an experimental run of deleting v1 embedding.

This change is breaking as it drops support for old non-migrated apps.

@stuartmorgan
Copy link
Contributor

Is this intended for review at this stage? It's missing all the metadata updates.

@stuartmorgan
Copy link
Contributor

This change is breaking as it drops support for old non-migrated apps.

We need to think carefully about whether we want to consider this a breaking change from a versioning standpoint; making a breaking version change to ~all of our plugins has non-trivial ecosystem impacts.

@GaryQian
Copy link
Contributor Author

This is not yet meant to be merged/reviewed. I definitely expect more discussion on how to best proceed with the full deprecation of v1 embedding, especially with things like the plugin ecosystem.

I would welcome any feedback/thoughts on how to move forwards though!

Is there any way to run the framework tests with changes like this? Currently, many of the framework tests fail due to these deprecated registerWith methods.

@GaryQian
Copy link
Contributor Author

I think I'll opt for a non-breaking deprecation of the v1 registrant API by making the API a no-op + warning. This would be far less disruptive, especially for plugins that are no longer actively maintained. I'll leave this PR open for further experimentation and testing though.

@GaryQian GaryQian marked this pull request as draft October 28, 2021 23:38
@stuartmorgan
Copy link
Contributor

/cc @blasten

Is there any way to run the framework tests with changes like this? Currently, many of the framework tests fail due to these deprecated registerWith methods.

Are the framework tests that are failing referencing the plugins by path, or using the published versions? If the latter, you'd have to individually change all of them to git references.

I think I'll opt for a non-breaking deprecation of the v1 registrant API by making the API a no-op + warning. This would be far less disruptive, especially for plugins that are no longer actively maintained. I'll leave this PR open for further experimentation and testing though.

Isn't there already a deprecation warning? Why do we need a secondary warning on top of that?

@stuartmorgan
Copy link
Contributor

I would welcome any feedback/thoughts on how to move forwards though!

It feels like trying to push plugins to drop support for the v1 embedding is solving the wrong problem (which is another reason I don't think adding warnings is a good idea).

We don't support v1 in our plugins because we're behind, or because we don't know better, we're supporting v1 because we can't rely on clients having v2 support in their application (and I'm sure the same is true of much of the rest of the ecosystem). The place to fix it is on the demand side (applications) rather than the supply side (plugins).

I would recommend the following:

  • Make relying on v1 support in applications something that is no longer possible, along with whatever tooling support is needed to make that viable (e.g., fatal errors on build if people try).
  • Make a clear announcement that as of Flutter version X.Y, v1 embedding is non-functional. This should probably be in the release announcement, as well as documented on our wiki for people to easily find.
  • Annotate all of the code that this PR removes with a comment saying "Remove this code when this plugin requires a minimum version of X.Y".

Then plugin developers can make the change whenever makes sense for them based on their support range, and do so in a way that is non-breaking.

@GaryQian
Copy link
Contributor Author

GaryQian commented Nov 2, 2021

go/flutter-delete-v1-android

@godofredoc godofredoc changed the base branch from master to main January 6, 2022 22:47
@GaryQian GaryQian closed this Jan 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants