-
Notifications
You must be signed in to change notification settings - Fork 945
Fix some incorrect typings / missing pieces in the new auth SDK #4187
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
Conversation
|
Size Analysis ReportAffected Products
|
Type | Base (2b8a03b) | Head (e94a586) | Diff |
---|---|---|---|
size | 31.8 kB | 32.3 kB | +481 B (+1.5%) |
size_with_ext_deps | 42.3 kB | 42.8 kB | +484 B (+1.1%) |
GithubAuthProvider
Size
Type | Base (2b8a03b) | Head (e94a586) | Diff |
---|---|---|---|
size | 31.8 kB | 32.3 kB | +481 B (+1.5%) |
size_with_ext_deps | 42.3 kB | 42.8 kB | +484 B (+1.1%) |
GoogleAuthProvider
Size
Type | Base (2b8a03b) | Head (e94a586) | Diff |
---|---|---|---|
size | 31.8 kB | 32.3 kB | +481 B (+1.5%) |
size_with_ext_deps | 42.3 kB | 42.8 kB | +484 B (+1.1%) |
OAuthProvider
Size
Type | Base (2b8a03b) | Head (e94a586) | Diff |
---|---|---|---|
size | 31.2 kB | 31.7 kB | +481 B (+1.5%) |
size_with_ext_deps | 41.7 kB | 42.2 kB | +484 B (+1.2%) |
TwitterAuthProvider
Size
Type | Base (2b8a03b) | Head (e94a586) | Diff |
---|---|---|---|
size | 31.8 kB | 32.3 kB | +481 B (+1.5%) |
size_with_ext_deps | 42.3 kB | 42.8 kB | +484 B (+1.1%) |
browserPopupRedirectResolver
Size
Type | Base (2b8a03b) | Head (e94a586) | Diff |
---|---|---|---|
size | 45.3 kB | 45.8 kB | +481 B (+1.1%) |
size_with_ext_deps | 55.9 kB | 56.4 kB | +484 B (+0.9%) |
getAuth
Size
Type | Base (2b8a03b) | Head (e94a586) | Diff |
---|---|---|---|
size | 55.5 kB | 55.9 kB | +465 B (+0.8%) |
size_with_ext_deps | 66.8 kB | 66.7 kB | -149 B (-0.2%) |
External Dependencies
Module | Base (2b8a03b) | Head (e94a586) | Diff |
---|---|---|---|
@firebase/app-exp |
SDK_VERSION _getProvider _registerComponent getApp registerVersion |
SDK_VERSION _getProvider _registerComponent registerVersion |
- getApp |
initializeAuth
Size
Type | Base (2b8a03b) | Head (e94a586) | Diff |
---|---|---|---|
size | 28.8 kB | 28.8 kB | -18 B (-0.1%) |
size_with_ext_deps | 40.0 kB | 39.4 kB | -631 B (-1.6%) |
External Dependencies
Module | Base (2b8a03b) | Head (e94a586) | Diff |
---|---|---|---|
@firebase/app-exp |
SDK_VERSION _getProvider _registerComponent getApp registerVersion |
SDK_VERSION _getProvider _registerComponent registerVersion |
- getApp |
linkWithPopup
Size
Type | Base (2b8a03b) | Head (e94a586) | Diff |
---|---|---|---|
size | 36.9 kB | 37.4 kB | +481 B (+1.3%) |
size_with_ext_deps | 47.4 kB | 47.9 kB | +484 B (+1.0%) |
linkWithRedirect
Size
Type | Base (2b8a03b) | Head (e94a586) | Diff |
---|---|---|---|
size | 32.0 kB | 32.4 kB | +481 B (+1.5%) |
size_with_ext_deps | 42.5 kB | 43.0 kB | +484 B (+1.1%) |
reauthenticateWithPopup
Size
Type | Base (2b8a03b) | Head (e94a586) | Diff |
---|---|---|---|
size | 36.9 kB | 37.4 kB | +481 B (+1.3%) |
size_with_ext_deps | 47.5 kB | 47.9 kB | +484 B (+1.0%) |
reauthenticateWithRedirect
Size
Type | Base (2b8a03b) | Head (e94a586) | Diff |
---|---|---|---|
size | 31.7 kB | 32.2 kB | +481 B (+1.5%) |
size_with_ext_deps | 42.2 kB | 42.7 kB | +484 B (+1.1%) |
setPersistence
Size
Type | Base (2b8a03b) | Head (e94a586) | Diff |
---|---|---|---|
size | 28.5 kB | 28.5 kB | +7 B (+0.0%) |
size_with_ext_deps | 39.0 kB | 39.0 kB | +7 B (+0.0%) |
signInWithPopup
Size
Type | Base (2b8a03b) | Head (e94a586) | Diff |
---|---|---|---|
size | 36.9 kB | 37.4 kB | +481 B (+1.3%) |
size_with_ext_deps | 47.4 kB | 47.9 kB | +484 B (+1.0%) |
signInWithRedirect
Size
Type | Base (2b8a03b) | Head (e94a586) | Diff |
---|---|---|---|
size | 31.4 kB | 31.9 kB | +481 B (+1.5%) |
size_with_ext_deps | 42.0 kB | 42.4 kB | +484 B (+1.2%) |
Test Logs
- Base (
2b8a03b7
): https://github.com/firebase/firebase-js-sdk/actions/runs/409265205 - Head (
e94a5868
): https://github.com/firebase/firebase-js-sdk/actions/runs/411268191
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.
While you're in here making changes can we fix getAuth()
to require the app
argument? According to @Feiyang1 it should be required.
Okay @samtstern, I've added the fixes for |
Specifically, this PR:
setPersistence
to be the correct value (Promise<void>
) in the typings packageOAuthProvider.credentialFromResult()
andOAuthProvider.credentialFromError()