-
Notifications
You must be signed in to change notification settings - Fork 4k
[firebase_auth] Correct method signature for confirmPasswordReset #1780
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
[firebase_auth] Correct method signature for confirmPasswordReset #1780
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we should avoid changing the method signature of confirmPasswordReset
because it would be a breaking change, and we are trying to avoid breaking changes to platform interfaces where possible. What I'd suggest instead is that we add app
as an optional named parameter to confirmPasswordReset
.
Future<void> confirmPasswordReset(String oobCode, String newPassword, { String app });
@hterkelsen if you feel otherwise please chime in
@collinjackson I understand where you're coming from from but |
I agree with @bgetsug. I should have caught this in code review yesterday. Since this hasn't even landed in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with making an exception here and allowing the breaking change, but I think it should be noted in the changelog.
packages/firebase_auth/firebase_auth_platform_interface/CHANGELOG.md
Outdated
Show resolved
Hide resolved
…LOG.md Co-Authored-By: Collin Jackson <[email protected]>
…rebase#1780) * add 'app' to signature of FirebaseAuthPlatform.confirmPasswordReset * Update packages/firebase_auth/firebase_auth_platform_interface/CHANGELOG.md Co-authored-by: Collin Jackson <[email protected]>
Description
The
FirebaseAuthPlatform.confirmPasswordReset
method should acceptString app
as an argument to be consistent with the rest of the interface (and existing implementations, particularly forfirebase_auth_web
).cc: @lanmonster, @hterkelsen
Related Issues
#1778
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
). This will ensure a smooth and quick review process.///
).flutter analyze
) does not report any problems on my PR.Breaking Change
Does your PR require plugin users to manually update their apps to accommodate your change?