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(authentication): add getIdTokenResult method #786

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

josephgodwinkimani
Copy link

@josephgodwinkimani josephgodwinkimani commented Dec 28, 2024

Pull request checklist

Please check if your PR fulfills the following requirements:

  • The changes have been tested successfully.
  • A changeset has been created (npm run changeset).
  • I have read and followed the pull request guidelines.

Copy link
Member

@robingenz robingenz left a comment

Choose a reason for hiding this comment

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

Please add the missing type GetIdTokenResultOptions, create a changeset (npm run changeset) and test your implementation.

Copy link
Author

@josephgodwinkimani josephgodwinkimani left a comment

Choose a reason for hiding this comment

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

add the missing type GetIdTokenResultOptions

@robingenz
Copy link
Member

@josephgodwinkimani Please create a changeset using npm run changeset.

@robingenz
Copy link
Member

Thank you, i will merge it this week.

Copy link
Member

@robingenz robingenz left a comment

Choose a reason for hiding this comment

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

I have just noticed that support for Android and iOS is missing. Please also add this since this plugin supports all 3 platforms.

@robingenz
Copy link
Member

@josephgodwinkimani Let me know as soon as you're done and I can review it again.

@josephgodwinkimani
Copy link
Author

@josephgodwinkimani Let me know as soon as you're done and I can review it again.

@robingenz kindly review my latest commit

@robingenz
Copy link
Member

@josephgodwinkimani Please fix the build and lint errors and resolve the merge conflict.

@josephgodwinkimani
Copy link
Author

josephgodwinkimani commented Jan 23, 2025

@josephgodwinkimani Please fix the build and lint errors and resolve the merge conflict.

@robingenz I can see a notice of the conflict in packages/authentication/ios/Plugin/FirebaseAuthenticationPlugin.m, but I am unable to identify the specific nature of the conflict.

@robingenz
Copy link
Member

robingenz commented Jan 31, 2025

@josephgodwinkimani I cannot merge this as long as you have this merge conflict. Just delete the FirebaseAuthenticationPlugin.m file. It no longer exists. You need to register your method in the FirebaseAuthenticationPlugin.swift file.

@robingenz
Copy link
Member

@josephgodwinkimani Your build failed. Please teste your changes before requesting a review.

@josephgodwinkimani
Copy link
Author

@josephgodwinkimani Your build failed. Please teste your changes before requesting a review.

Thank you for pointing out the build failure. Unfortunately, I do not have access to an Apple-based machine to test iOS-specific changes locally. I recommend that someone with the appropriate hardware and environment verify the build. Please let me know if there's anything else I can assist with in resolving this issue.

@robingenz
Copy link
Member

@josephgodwinkimani Alright, I will take a look the iOS implementation. Please make sure to test the Android implementation.

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.

2 participants