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

feat(auth): Add Expo support for phone auth #6645

Merged

Conversation

DoctorJohn
Copy link
Contributor

@DoctorJohn DoctorJohn commented Oct 29, 2022

Description

I rebased #6167 on main after #6472 stalled.
I also added a missing info plist value check and removed a duplicated app.json config key existance check.
Also added some tests for the config plugin. Let's see whether this makes CI happy :)

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

Compared plugin with official setup instructions for iOS and confirmed that Android doesn't need any extra steps.
Also added some tests to verify the plugin does what it's supposed to do.

🔥 :)


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

@vercel
Copy link

vercel bot commented Oct 29, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
react-native-firebase ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Oct 30, 2022 at 4:15PM (UTC)
1 Ignored Deployment
Name Status Preview Comments Updated
react-native-firebase-next ⬜️ Ignored (Inspect) Oct 30, 2022 at 4:15PM (UTC)

@codecov
Copy link

codecov bot commented Oct 29, 2022

Codecov Report

Merging #6645 (13e489b) into main (1f2385b) will increase coverage by 0.14%.
The diff coverage is 89.48%.

❗ Current head 13e489b differs from pull request most recent head 006b757. Consider uploading reports for the commit 006b757 to get more accurate results

@@            Coverage Diff             @@
##             main    #6645      +/-   ##
==========================================
+ Coverage   72.20%   72.33%   +0.14%     
==========================================
  Files         115      116       +1     
  Lines        4754     4792      +38     
  Branches     1064     1072       +8     
==========================================
+ Hits         3432     3466      +34     
- Misses       1240     1244       +4     
  Partials       82       82              

Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

I just bumped the config plugin dep to v5 (along with a trivial forward port for the same) to match deps from #6482 and force pushed it to get CI through - should be good to go from that

Thanks for rescuing the PR and getting it through

@barthap
Copy link
Contributor

barthap commented Oct 30, 2022

Great to see that you took care of this feature! 😀 After a brief look it looks good to me, glad you added unit tests 😉

@mikehardy
Copy link
Collaborator

@barthap I always appreciate the support on the Expo stuff - highly valued - thanks!

@mikehardy
Copy link
Collaborator

Almost unbelievably we still have a transitive dependency on the deprecated jcenter repository and they must be doing an unscheduled brownout (to surface these deps...) which failed the android build


* What went wrong:
Execution failed for task ':jet:dexBuilderDebugAndroidTest'.
> Could not resolve all files for configuration ':jet:debugRuntimeClasspath'.
   > Could not resolve com.facebook.react:react-native:+.
     Required by:
         project :jet
      > Failed to list versions for com.facebook.react:react-native.
         > Unable to load Maven meta-data from https://jcenter.bintray.com/com/facebook/react/react-native/maven-metadata.xml.
            > Could not HEAD 'https://jcenter.bintray.com/com/facebook/react/react-native/maven-metadata.xml'.
               > Read timed out

I just fixed it I think with https://github.com/invertase/jet/releases/tag/v0.8.2 but it is not worth pushing in this PR to fix CI because the PR is ios + javascript only and those checks passed.

@mikehardy mikehardy merged commit 97a4ea5 into invertase:main Oct 30, 2022
@mikehardy mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label Oct 30, 2022
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.

4 participants