Skip to content

Fix a bunch of little issues with the new auth API #3999

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Dec 3, 2020
Merged

Fix a bunch of little issues with the new auth API #3999

merged 8 commits into from
Dec 3, 2020

Conversation

sam-gc
Copy link
Contributor

@sam-gc sam-gc commented Oct 26, 2020

No description provided.

@sam-gc sam-gc requested a review from avolkovi October 26, 2020 20:16
@changeset-bot
Copy link

changeset-bot bot commented Oct 26, 2020 •
edited
Loading

⚠️ No Changeset found

Latest commit: f841fb2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

avolkovi
avolkovi reviewed Oct 26, 2020
View reviewed changes
docs-exp/auth-types.actioncodeinfo.data.md Outdated

For the [Operation.REVERT\_SECOND\_FACTOR\_ADDITION](./auth-types.operation.revert_second_factor_addition.md) action, which allows a user to unenroll a newly added second factor, this object contains a `multiFactorInfo` field with the information about the second factor. For phone second factor, the `multiFactorInfo` is a [MultiFactorInfo](./auth-types.multifactorinfo.md) object, which contains the phone number.
For the action, which allows a user to unenroll a newly added second factor, this object contains a `multiFactorInfo` field with the information about the second factor. For phone second factor, the `multiFactorInfo` is a [MultiFactorInfo](./auth-types.multifactorinfo.md) object, which contains the phone number.
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like a broken @link in the docs

avolkovi
avolkovi previously requested changes Oct 26, 2020
View reviewed changes
Copy link
Contributor

@avolkovi avolkovi left a comment

Choose a reason for hiding this comment

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

Can you also pull in the changes from #3990 so I can close that PR

avolkovi
avolkovi reviewed Oct 26, 2020
View reviewed changes
packages-exp/auth-exp/src/core/providers/oauth.ts Outdated
@@ -24,7 +24,7 @@ import { OAuthCredential } from '../credentials/oauth';

export type CustomParameters = Record<string, string>;

interface CredentialParameters {
export interface OAuthCredentialOptions {
Copy link
Contributor

Choose a reason for hiding this comment

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

should this live in auth-types-exp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no; this is an internal-only interface

avolkovi
avolkovi reviewed Oct 26, 2020
View reviewed changes
packages-exp/auth-exp/src/core/user/additional_user_info.test.ts
@@ -35,13 +31,13 @@ import { User, UserCredential } from '../../model/user';
import { testAuth, testUser } from '../../../test/helpers/mock_auth';

describe('core/user/additional_user_info', () => {
const userProfileWithLogin: UserProfile = {
const userProfileWithLogin: Record<string, unknown> = {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this type necessary here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes; object literals should have a strong type specified

@sam-gc sam-gc force-pushed the samgho/auth-match branch from fada3a0 to 3de3868 Compare December 2, 2020 21:26
@sam-gc sam-gc requested a review from yuchenshi as a code owner December 2, 2020 21:26
@sam-gc sam-gc force-pushed the samgho/auth-match branch from e3935d9 to f841fb2 Compare December 3, 2020 16:55
@sam-gc sam-gc requested a review from avolkovi December 3, 2020 16:56
@hsubox76 hsubox76 dismissed avolkovi’s stale review December 3, 2020 17:08

Reviewer on leave.

@google-oss-bot
Copy link
Contributor

Size Analysis Report

Affected Products

@firebase/auth-exp

  • PhoneMultiFactorGenerator

    Size

    Type Base (f2562c2) Head (02480d1) Diff
    size 29.4 kB 29.4 kB -5 B (-0.0%)
    size_with_ext_deps 39.9 kB 39.9 kB -5 B (-0.0%)

  • browserPopupRedirectResolver

    Size

    Type Base (f2562c2) Head (02480d1) Diff
    size 45.3 kB 45.3 kB -28 B (-0.1%)
    size_with_ext_deps 55.9 kB 55.9 kB -28 B (-0.1%)

  • getAuth

    Size

    Type Base (f2562c2) Head (02480d1) Diff
    size 55.5 kB 55.5 kB -28 B (-0.1%)
    size_with_ext_deps 66.8 kB 66.8 kB -28 B (-0.0%)

  • getMultiFactorResolver

    Size

    Type Base (f2562c2) Head (02480d1) Diff
    size 31.3 kB 31.2 kB -35 B (-0.1%)
    size_with_ext_deps 41.8 kB 41.7 kB -36 B (-0.1%)

  • getRedirectResult

    Size

    Type Base (f2562c2) Head (02480d1) Diff
    size 34.1 kB 34.1 kB +7 B (+0.0%)
    size_with_ext_deps 44.6 kB 44.6 kB +7 B (+0.0%)

  • linkWithPopup

    Size

    Type Base (f2562c2) Head (02480d1) Diff
    size 36.9 kB 36.9 kB -28 B (-0.1%)
    size_with_ext_deps 47.5 kB 47.4 kB -28 B (-0.1%)

  • reauthenticateWithCredential

    Size

    Type Base (f2562c2) Head (02480d1) Diff
    size 30.1 kB 30.1 kB -28 B (-0.1%)
    size_with_ext_deps 40.6 kB 40.6 kB -28 B (-0.1%)

  • reauthenticateWithPhoneNumber

    Size

    Type Base (f2562c2) Head (02480d1) Diff
    size 33.6 kB 33.6 kB -28 B (-0.1%)
    size_with_ext_deps 44.1 kB 44.1 kB -28 B (-0.1%)

  • reauthenticateWithPopup

    Size

    Type Base (f2562c2) Head (02480d1) Diff
    size 37.0 kB 36.9 kB -28 B (-0.1%)
    size_with_ext_deps 47.5 kB 47.5 kB -28 B (-0.1%)

  • signInWithCredential

    Size

    Type Base (f2562c2) Head (02480d1) Diff
    size 29.9 kB 29.9 kB -28 B (-0.1%)
    size_with_ext_deps 40.4 kB 40.4 kB -28 B (-0.1%)

  • signInWithEmailAndPassword

    Size

    Type Base (f2562c2) Head (02480d1) Diff
    size 33.3 kB 33.3 kB -28 B (-0.1%)
    size_with_ext_deps 43.9 kB 43.8 kB -28 B (-0.1%)

  • signInWithEmailLink

    Size

    Type Base (f2562c2) Head (02480d1) Diff
    size 33.5 kB 33.5 kB -28 B (-0.1%)
    size_with_ext_deps 44.0 kB 44.0 kB -28 B (-0.1%)

  • signInWithPhoneNumber

    Size

    Type Base (f2562c2) Head (02480d1) Diff
    size 33.4 kB 33.4 kB -28 B (-0.1%)
    size_with_ext_deps 43.9 kB 43.9 kB -28 B (-0.1%)

  • signInWithPopup

    Size

    Type Base (f2562c2) Head (02480d1) Diff
    size 36.9 kB 36.9 kB -28 B (-0.1%)
    size_with_ext_deps 47.5 kB 47.4 kB -28 B (-0.1%)

Test Logs

@sam-gc sam-gc merged commit 93d8dc0 into master Dec 3, 2020
@sam-gc sam-gc deleted the samgho/auth-match branch December 3, 2020 17:26
@firebase firebase locked and limited conversation to collaborators Jan 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hsubox76 hsubox76 hsubox76 approved these changes

@Feiyang1 Feiyang1 Awaiting requested review from Feiyang1

@hiranya911 hiranya911 Awaiting requested review from hiranya911

@yuchenshi yuchenshi Awaiting requested review from yuchenshi

@avolkovi avolkovi Awaiting requested review from avolkovi

Assignees

@avolkovi avolkovi

@sam-gc sam-gc

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sam-gc @google-oss-bot @avolkovi @hsubox76