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(auth, ios): factorId nil check #7541

Merged
merged 6 commits into from
Jan 6, 2024

Conversation

vargajacint
Copy link
Contributor

Description

Sometimes the hint.factorID (in the firebaseUserToDict function) is retuning nil value and this could cause app crash. I've tried to described my journey with this issue in this comment

Related issues

Fixes #7080

Release Summary

Auth module - nil check in the getJSFactorId function

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

🔥

Copy link

vercel bot commented Dec 27, 2023

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

Name Status Preview Comments Updated (UTC)
react-native-firebase ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 6, 2024 7:31pm
react-native-firebase-next 🛑 Canceled (Inspect) Jan 6, 2024 7:31pm

@CLAassistant
Copy link

CLAassistant commented Dec 27, 2023

CLA assistant check
All committers have signed the CLA.

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.

Hey - thanks a bunch for working through this. I'm sorry it is crashing for you and I'm certain we need a change here so I really appreciate the investigation

I've let some comments on the specific direction for the change and I'm curious for your thoughts on the comments - after a little conversation between us I'm sure we'll come up with something that we agree on and we can get a fix released for this

packages/auth/ios/RNFBAuth/RNFBAuthModule.m Outdated Show resolved Hide resolved
packages/auth/ios/RNFBAuth/RNFBAuthModule.m Outdated Show resolved Hide resolved
packages/auth/ios/RNFBAuth/RNFBAuthModule.m Outdated Show resolved Hide resolved
@mikehardy mikehardy added the Workflow: Waiting for User Response Blocked waiting for user response. label Dec 29, 2023
@vargajacint
Copy link
Contributor Author

Hey @mikehardy,
Yeah you were right, I've applied your suggested changes! Thank you, and happy new year!

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.

This looks clean and easy to me - I'm a +1 on it - does it work for you to solve the crash in testing?

@mikehardy mikehardy added Workflow: Pending Merge Waiting on CI or similar and removed Workflow: Waiting for User Response Blocked waiting for user response. labels Jan 6, 2024
@mikehardy
Copy link
Collaborator

Already passed iOS, android code didn't change --> good to go

@mikehardy mikehardy merged commit b1cee9a into invertase:main Jan 6, 2024
15 of 17 checks passed
@mikehardy mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label Jan 6, 2024
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.

[🐛] [iOS] [Auth] - The app crash on iOS when I login.
3 participants