-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[many] Remove references to v1 embedding #6494
Conversation
cc @stuartmorgan I was unaware that we had recommended folks include the static This means that the deletion of the v1 embedding will be a breaking change for most plugins, though the solution is simply to remove that static method. Do you have any opinions on: how to make this the least painful/would you LGTM the deletion in the first place/would you want to keep the interface needed around, so we can have a sort of no-op |
It's been a while since I was in conversations around the v1 wind-down, so I don't have all the context any more. What's the current state of v1 support on The way I would want this to go is:
I think I'd like to keep |
Currently still possible to build a v1 app by passing a flag (will error without the flag), though that will change in the next release (once flutter/flutter#144726 gets picked up). For full transparency we have been attempting to approach it faster, as a flat out deletion rather than wind down, given how long the v1 has been deprecated at this point. But this particular case seems like it will require more messaging before deletion is possible, given our own recommendations put plugin authors in this situation.
To be a little nitpicky, But I'm fine leaving that interface around (with a comment warning it is slated for deletion) for a release to give plugin authors time to migrate, while deleting the rest of the v1 embedding now. Does that sound reasonable to you? Also I'm happy to help reach out to the top plugin authors, if we have a list somewhere. |
Also in light of
I'll get this pr to tests passing, and then leave it around until the next release before requesting review. |
Right, I was thinking of the entire process of "fully supported" to "completely gone" when I say wind-down. I've been involved in discussions on and off since very early in the process.
Ah, that's more unfortunate in terms of code volume. But since it's just an interface we could strip out all the comments and at least get it down to a pretty small amount.
Yes, I'm on board with that. The main thing I had pushed back on in the past was deleting the v1 support from our plugins while the tool still supported v1 builds, because it seemed backward. It's much smoother if we delete things from the tool up (just as we do with dropping old OS versions).
I'll get an updated list and do the outreach once the change hits |
flutter/engine#51229 blocked [the roll](#146522) and had to be reverted, which is a shame, but on the bright side it made it possible to point the framework at my removal pr, at the point of its merging the first time This fixes all errors that are fixable in the framework that would have blocked the roll. There are some that aren't fixable here (they need to be fixed in the engine)*, so I'll fix those in the engine but unfortunately I can't pick up another version here to re-test until I try to roll again � *This category is: uses of plugins that in turn have a `registerWith`, that references the v1 embedding. The plan to fix these cases is to leave the interface that that method relies on around for now. See flutter/packages#6494 (comment) for details
I'm not sure I understand why we aren't landing this PR now.
Why are we keeping dead and untested code around in our repository? |
According to this comment it's been removed on
Not having tests for v1 here doesn't mean we can intentionally break v1 support. It's in the same category as "best effort" OS support.
Can you elaborate on what "broken" means, concretely? What happens, right now, if someone attempts to build an app on |
This is an API that was deprecated 5+ years ago and has no users.
The app will build but fail at runtime. |
Both of these things are completely new information to me. Is there more information on this somewhere? |
#6923 is a follow up pr that includes more migration steps. Both will be required to build after v1 embedding classes are removed.
I have a WIP effort to remove the v1 embedding entirely (flutter/flutter#146523 is the latest pr).
registerWith
references the v1 embedding, which has been deprecated for many years, so this PR removes it from all plugins.Also removes some additional references, see in particular these three commits:
ActivityState
class to removePluginRegistry.Registrar
member inimage_picker
: c2e4c87FlutterMain.getLookupKeyForAsset()
withFlutterLoader.getLookupKeyForAsset()
ingoogle_maps
plugin: 73c3de3RegistrarFlutterAssetManager
class inwebview_flutter
: cc842c6Fixes flutter/flutter#70923
Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.