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

fix androidXVersion to prevent build fails #399

Merged
merged 2 commits into from
Nov 1, 2023

Conversation

IgorVanian
Copy link
Contributor

PR Checklist

What is the current behavior?

The investigation is available here: #398

This exact problem was already addressed multiple times and we still get surprised by failing builds (#298, #386 ). I think it's worth to set a fixed version to prevent this kind of surprise in the future. 🙏

What is the new behavior?

Fixes androidXVersion to 1.5.+ which is the latest before the kotlin 1.8.0 issue...

Fixes/Implements/Closes #398.

@@ -58,7 +58,7 @@ dependencies {
implementation "com.android.support:support-annotations:$supportLibVersion"
implementation "com.android.support:customtabs:$supportLibVersion"
} else {
def defaultAndroidXVersion = "1.+"
def defaultAndroidXVersion = "1.5.+"
Copy link

@RyanThomas73 RyanThomas73 Jan 27, 2023

Choose a reason for hiding this comment

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

@IgorVanian
I think you'll need to define separate defaults to do this since androidXAnnotation and androidXBrowser don't have the same stable version numbers.

The latest stable on androidx.browser is 1.4.0.

e.g.

def defaultAndroidXAnnotationVersion = "1.5.+"
def defaultAndroidXBrowserVersion = "1.4.+"
if (androidXVersion != null) {
  defaultAndroidXAnnotationVersion = androidXVersion
  defaultAndroidXBrowserVersion = androidXVersion
}
def androidXAnnotation = safeExtGet('androidXAnnotation', defaultAndroidXAnnotationVersion)
def androidXBrowser = safeExtGet('androidXBrowser', defaultAndroidXBrowserVersion)

NOTES:
Personally I don't think it should be trying to default them both to a shared androidXVersion since androidx packages versions are independent of each other.

But removing that part would be a breaking change and require a major version bump so I left it in my snippet for now.

Copy link
Contributor Author

@IgorVanian IgorVanian Jan 30, 2023

Choose a reason for hiding this comment

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

You're right, there are separate versions in AndroidX for both. I don't see how this is a breaking change though? They are versions that were used until now anyway. What broke recently was actually a bump in AndroidX releases.

By the way, I don't even understand why should we be able to set a specific version for something that is required by the lib. If there are version differences in dependencies, Gradle will resolve the conflict by using the latest one anyway. We could just set the minimum required version in the library.

Copy link

@RyanThomas73 RyanThomas73 Jan 30, 2023

Choose a reason for hiding this comment

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

I was saying that - long term this part should go away

if (androidXVersion != null) {
  defaultAndroidXAnnotationVersion = androidXVersion
  defaultAndroidXBrowserVersion = androidXVersion
}

BUT removing that part specifically would be a breaking change because if someone currently using the library has that set and we start ignoring it would change the behavior they experience.

@felipecamposfabel
Copy link

Hi. Any plans for merging this commit? A new androidX version 1.6 is out and started to cause problems with SDK 33 now.
https://developer.android.com/jetpack/androidx/releases/browser.

I patched it on my project and seems to be working just fine on RN 0.71.7.

Thanks!

Copy link
Member

@jdnichollsc jdnichollsc left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your contribution! <3

@jdnichollsc jdnichollsc merged commit 624bfa7 into proyecto26:develop Nov 1, 2023
@jenskuhrjorgensen
Copy link

jenskuhrjorgensen commented Nov 2, 2023

Awesome, @jdnichollsc and @IgorVanian
Is there a planned new release with this fix anytime soon?

@jdnichollsc
Copy link
Member

@jenskuhrjorgensen I'm waiting for merging this PR in order to publish a new release #421

@IgorVanian IgorVanian deleted the fix-androidx-version branch November 6, 2023 08:30
@jenskuhrjorgensen
Copy link

@jdnichollsc Can we get a release containing this PR? It looks like the other PR died somewhere along the way.

@nikinsaw
Copy link

nikinsaw commented Aug 30, 2024

Hi, @jdnichollsc when can we expect a new release containing this PR?

@HacerBusraKILIC
Copy link

Hi @jdnichollsc, when will the new version containing the fix be released?

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

Successfully merging this pull request may close these issues.

New androidXAnnotation version breaks Android builds
8 participants