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(android, dependencies): remove vestigial ml dependencies, add browser dep to auth #4873

Merged
merged 3 commits into from
Feb 5, 2021

Conversation

mikehardy
Copy link
Collaborator

Description

Android dependencies in ML were still pulling in an on-device library, unnecessarily bloating APKs, #4750 / #4871

Auth module depends on androidx.browser for reCAPTCHA flow but it is not included transitively from upstream, this leads to crashes so including a workaround here while upstream issue resolves seems reasonable, #4744 / firebase/firebase-android-sdk#2164

Release Summary

This should be re-base merged, each commit has a correct commit title

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • Yes
  • My change supports the following platforms;
    • Android
    • iOS
  • My change includes tests;
    • e2e tests added or updated in packages/\*\*/e2e
    • jest tests added or updated in packages/\*\*/__tests__
  • I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • No

Test Plan


Think react-native-firebase is great? Please consider supporting the project with any of the below:

The reCAPTCHA auth flow requires androidx.browser but it is not specified
as a transitive dependency upstream, so it crashes.

I attempted to use as wide a version range as I felt safe, and provided an
override if you specify "androidxBrowserVersion" in your android/build.gradle
ext config block

Related #4744
Related firebase/firebase-android-sdk#2164

This may be removed when the upstream issue is closed.
image labeling on device is no longer supported but we were still
referencing some on-device dependencies, causing the built APKs to be
larger than necessary when they included the underlying native libraries

Fixes #4750
@vercel
Copy link

vercel bot commented Feb 5, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/emcysni9v
✅ Preview: https://react-native-firebase-git-mikehardy-android-dependency-fixes.invertase.vercel.app

@mikehardy mikehardy added the Workflow: Pending Merge Waiting on CI or similar label Feb 5, 2021
@codecov
Copy link

codecov bot commented Feb 5, 2021

Codecov Report

Merging #4873 (4617c0d) into master (d17b382) will decrease coverage by 52.20%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master    #4873       +/-   ##
===========================================
- Coverage   89.06%   36.87%   -52.19%     
===========================================
  Files         109       48       -61     
  Lines        3720     1454     -2266     
  Branches      347      347               
===========================================
- Hits         3313      536     -2777     
- Misses        365      697      +332     
- Partials       42      221      +179     

…minimum

3.5.3, which comes default with react-native@0.63.x still, has AAPT failures
@mikehardy mikehardy force-pushed the @mikehardy/android-dependency-fixes branch from 318fcff to 707631d Compare February 5, 2021 19:13
@mikehardy mikehardy merged commit 077ca0f into master Feb 5, 2021
@mikehardy mikehardy deleted the @mikehardy/android-dependency-fixes branch February 5, 2021 19:20
@mikehardy
Copy link
Collaborator Author

also snuck a note in there about in-app-messaging needing android gradle plugin 3.5.4 as a minimum, discovered in testing all of these

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.

1 participant