-
Notifications
You must be signed in to change notification settings - Fork 4k
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
refactor: Migrate firebase auth to nnbd #4633
Conversation
|
/// Complete error message. | ||
final String message; | ||
FirebaseAuthException({ | ||
String? message, |
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.
"message" is optional/nullable in base-classes, and some places passed null
as message, so I removed the required
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.
required
removal makes sense 👍
if (exception is! Exception || exception is! PlatformException) { | ||
// TODO(rrousselGit): Is this dead code? | ||
return exception; |
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.
This code cannot be assigned to Exception
, so I changed the return type to Object
.
But that raise a question: Why do we even need this code? It probably was never reached before, as we'd otherwise get a "XX cannot be assigned to Exception" thrown
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 can't think of any reason why this was needed, @Ehesp might be able to confirm if around
@@ -10,13 +10,13 @@ import '../../firebase_auth_exception.dart'; | |||
|
|||
/// Catches a [PlatformException] and converts it into a [FirebaseAuthException] | |||
/// if it was intentionally caught on the native platform. | |||
Exception convertPlatformException(Object exception) { | |||
Object convertPlatformException(Object exception) { | |||
if (exception is! Exception || exception is! PlatformException) { |
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.
PlatformException is an Exception so the first condition isn't needed
String /*!*/ message = platformException.message; | ||
String email; | ||
AuthCredential credential; | ||
String? message = platformException.message; |
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.
Related to the previous comment about message
now being nullable
I believe the /*!*/
was erroneous
|
||
final additionalUserInfo = result.additionalUserInfo; | ||
expect(additionalUserInfo, isA<Object>()); | ||
expect(additionalUserInfo, isNull); |
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.
isA<Object>
is always true. Using isNull
since that is the current behaviour, but could be wrong
verificationCompleted: (_) {}, | ||
verificationFailed: (_) {}, | ||
codeSent: (_, __) {}, | ||
codeAutoRetrievalTimeout: (_) {}, | ||
timeout: testTimeout, | ||
autoRetrievedSmsCodeForTesting: testSmsCode, |
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.
Not sure if this change is expected, or if some parameters could be nullable?
String type, | ||
void Function() testMethod, | ||
) async { | ||
await expectLater( |
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.
Same behaviour, but much better error message
There are many tests that should be changed to follow this pattern
.add(DivElement()..id = _kInvisibleElementId); | ||
|
||
element = _kInvisibleElementId; | ||
} else { | ||
parameters['size'] = convertRecaptchaVerifierSize(size); | ||
parameters['theme'] = convertRecaptchaVerifierTheme(theme); | ||
|
||
Element el = window.document.getElementById(container); |
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.
It didn't feel correct to do nothing with el
besides an assertion. This change no-longer calls getElementById
when the asserts are removed
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.
Agree 👍
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
@googlebot I consent. |
d324b34
to
166eba2
Compare
1aab645
to
fd784b0
Compare
fd784b0
to
eba3d64
Compare
4d635d6
to
004da73
Compare
Description
This migrate firebase_auth to non-nullable types
Do not merge before firebase_core is migrated
Related Issues
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. Updating the
pubspec.yaml
and changelogs is not required.///
).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?