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

Conversation

@cyanglaz
Copy link
Contributor

@cyanglaz cyanglaz commented Jul 10, 2019

This reverts commit 19bdcce.

PR #844

@ened The previous PR broke some example apps in the repo because they have not been migrated to AndroidX yet. We have to keep this commit reverted until we migrate example apps to androidX

@cyanglaz cyanglaz requested a review from mehmetf as a code owner July 10, 2019 17:52
@cyanglaz cyanglaz requested a review from mklim July 10, 2019 17:52
Copy link
Contributor

@mklim mklim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I don't think the issue is with the example apps. It's more that while we have ongoing issues with AndroidX incompatibilities (flutter/flutter#23586) I'd rather not introduce new compile time dependencies on AndroidX in new plugins where we don't have to.

@cyanglaz cyanglaz merged commit d4cd9ec into flutter:master Jul 10, 2019
@cyanglaz cyanglaz deleted the revert branch July 10, 2019 18:04
@mklim
Copy link
Contributor

mklim commented Jul 10, 2019

Landed on red because the breakage is because of the version in pub breaking the example app, so CI would definitely be red until the fix is published. I tested this with the example app locally edited to use the filesystem instead of pub versions, and saw the revert fix the compile failure.

@ened
Copy link
Contributor

ened commented Jul 10, 2019

Ok no problem to revert.

I see two ways to fix it again:

  • depend on the legacy annotation library (AndroidX projects can use jetifier)
  • depend on AndroidX annotation library

Both times the actual google sign in library project would depend on it instead of the example.

A bit strange this landed on green and turned it red though, btw. Will prepare an updated PR with the first solution - @cyanglaz ok for you?

@mklim
Copy link
Contributor

mklim commented Jul 10, 2019

Thanks for understanding @ened and offering to help this re-land.

depend on the legacy annotation library (AndroidX projects can use jetifier)

Unfortunately Jetifier isn't working for Flutter plugins. It's incompatible with how Flutter plugins are built into Flutter Android apps. flutter/flutter#29328 is probably the most concise source of info on that issue right now. @blasten is working on fixing it, but until then the incompatibilities are unavoidably breaking.

Because of that I think it's better that we hold off on this until we get the plugins into a state where Jetifier is working. Right now there's no compile time support dependency in the plugin at all. Introducing either a legacy support or a new AndroidX dependency would cause incompatibilities in new cases where it's not strictly needed right now. Once Jetifier is working again I think this patch good to have, it does make sense for apps to move to a state where they're on AndroidX and using Jetifier so that they aren't hitting compatibility issues like this.

A bit strange this landed on green and turned it red though, btw.

Yeah, this is a side effect of how our CI is set up for these example apps. The example apps depend on the published versions of any secondary plugins they use. So in this case our CI didn't break until the new version was published and an example app that used it as a secondary dependency pulled the latest version in.

mithun-mondal pushed a commit to bKash-developer/archived_plugins that referenced this pull request Aug 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants