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.

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

Reorder RecaptchaVerifier params so auth is the first param #7326

Merged
merged 4 commits into from
Jul 5, 2023
Merged

Reorder RecaptchaVerifier params so auth is the first param #7326

merged 4 commits into from
Jul 5, 2023

Conversation

ch5zzy
Copy link
Contributor

@ch5zzy ch5zzy commented May 24, 2023

Reorder RecaptchaVerifier params so auth is the first param (b/273333245)

@changeset-bot
Copy link

changeset-bot bot commented May 24, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: ef0d4f9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@firebase/auth Major
@firebase/auth-compat Patch
firebase Major
@firebase/rules-unit-testing Major

Not sure what this means? Click here to learn what changesets are.

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

@google-oss-bot
Copy link
Contributor

google-oss-bot commented May 24, 2023 •
edited
Loading

Size Report 1

Affected Products

  • @firebase/auth

    TypeBase (fe7da7e)Merge (81a81ee)Diff
    browser170 kB171 kB+53 B (+0.0%)
    cordova198 kB198 kB-5 B (-0.0%)
    esm5223 kB223 kB+53 B (+0.0%)
    main167 kB168 kB+53 B (+0.0%)
    module170 kB171 kB+53 B (+0.0%)
    react-native183 kB183 kB-5 B (-0.0%)

  • @firebase/auth/cordova

    TypeBase (fe7da7e)Merge (81a81ee)Diff
    browser198 kB198 kB-5 B (-0.0%)
    module198 kB198 kB-5 B (-0.0%)

  • @firebase/auth/internal

    TypeBase (fe7da7e)Merge (81a81ee)Diff
    browser181 kB181 kB+53 B (+0.0%)
    esm5237 kB237 kB+53 B (+0.0%)
    main204 kB204 kB+53 B (+0.0%)
    module181 kB181 kB+53 B (+0.0%)

  • @firebase/auth/react-native

    TypeBase (fe7da7e)Merge (81a81ee)Diff
    browser183 kB183 kB-5 B (-0.0%)
    module183 kB183 kB-5 B (-0.0%)

  • @firebase/performance

    TypeBase (fe7da7e)Merge (81a81ee)Diff
    browser29.1 kB29.1 kB-1 B (-0.0%)
    esm531.0 kB31.0 kB-1 B (-0.0%)
    main31.5 kB31.4 kB-1 B (-0.0%)
    module29.1 kB29.1 kB-1 B (-0.0%)

  • bundle

    TypeBase (fe7da7e)Merge (81a81ee)Diff
    performance (trace)51.0 kB51.0 kB-1 B (-0.0%)

  • firebase

    TypeBase (fe7da7e)Merge (81a81ee)Diff
    firebase-auth-compat.js132 kB132 kB-5 B (-0.0%)
    firebase-auth-cordova.js147 kB147 kB-5 B (-0.0%)
    firebase-auth-react-native.js160 kB160 kB-5 B (-0.0%)
    firebase-auth.js127 kB127 kB+50 B (+0.0%)
    firebase-compat.js773 kB773 kB-6 B (-0.0%)
    firebase-performance-compat.js30.8 kB30.8 kB-1 B (-0.0%)
    firebase-performance-standalone-compat.es2017.js90.1 kB90.1 kB-1 B (-0.0%)
    firebase-performance-standalone-compat.js67.3 kB67.3 kB-1 B (-0.0%)
    firebase-performance.js30.8 kB30.8 kB-1 B (-0.0%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/5RnJtsjylK.html

@google-oss-bot
Copy link
Contributor

google-oss-bot commented May 24, 2023 •
edited
Loading

Size Analysis Report 1

Affected Products

  • @firebase/auth

    • TotpMultiFactorGenerator

      Size

      TypeBase (fe7da7e)Merge (81a81ee)Diff
      size37.5 kB37.6 kB+54 B (+0.1%)
      size-with-ext-deps58.3 kB58.4 kB+54 B (+0.1%)

    • multiFactor

      Size

      TypeBase (fe7da7e)Merge (81a81ee)Diff
      size37.3 kB37.3 kB-5 B (-0.0%)
      size-with-ext-deps58.1 kB58.1 kB-5 B (-0.0%)

  • @firebase/performance

    • getPerformance

      Size

      TypeBase (fe7da7e)Merge (81a81ee)Diff
      size18.0 kB18.0 kB-1 B (-0.0%)
      size-with-ext-deps50.9 kB50.9 kB-1 B (-0.0%)

    • initializePerformance

      Size

      TypeBase (fe7da7e)Merge (81a81ee)Diff
      size18.2 kB18.2 kB-1 B (-0.0%)
      size-with-ext-deps44.3 kB44.3 kB-1 B (-0.0%)

    • trace

      Size

      TypeBase (fe7da7e)Merge (81a81ee)Diff
      size17.9 kB17.9 kB-1 B (-0.0%)
      size-with-ext-deps43.7 kB43.7 kB-1 B (-0.0%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/hU8UqZKD6t.html

@ch5zzy ch5zzy requested a review from Xiaoshouzi-gh May 25, 2023 17:48
@ch5zzy ch5zzy marked this pull request as ready for review May 25, 2023 17:48
prameshj
prameshj reviewed May 30, 2023
View reviewed changes
Copy link
Contributor

@prameshj prameshj left a comment

Choose a reason for hiding this comment

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

One comment about compat, rest LGTM. Thanks!

@prameshj
Copy link
Contributor

prameshj commented Jun 6, 2023

We should also update external docs in https://firebase.google.com/docs/auth/web/phone-auth#set-up-the-recaptcha-verifier, which reference the snippets in https://github.com/firebase/snippets-web/blob/8fc7be2dc9ea398e63f57222d02428f8fa506a11/auth/phone-auth.js#L18-L24, once this is released.

@ch5zzy ch5zzy marked this pull request as draft June 6, 2023 19:00
@ch5zzy ch5zzy marked this pull request as ready for review June 6, 2023 21:53
prameshj
prameshj approved these changes Jun 7, 2023
View reviewed changes
Copy link
Contributor

@prameshj prameshj left a comment

Choose a reason for hiding this comment

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

Let's wait till June 19/20 to merge this, since it is a breaking change. FYI @hsubox76

@ch5zzy
Copy link
Contributor Author

ch5zzy commented Jun 7, 2023

I tested phone auth on both the auth and auth-compat demos to exercise the recaptcha flow and was able to sign in successfully with both.

@hsubox76 hsubox76 added this to the v10 milestone Jun 12, 2023
@ch5zzy ch5zzy mentioned this pull request Jun 13, 2023
Merged
@ch5zzy ch5zzy requested a review from hsubox76 June 14, 2023 17:05
Xiaoshouzi-gh
Xiaoshouzi-gh approved these changes Jun 26, 2023
View reviewed changes
Copy link

@Xiaoshouzi-gh Xiaoshouzi-gh left a comment

Choose a reason for hiding this comment

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

LGTM

@ch5zzy ch5zzy changed the title Reorder RecaptchaVerifier params so auth is the first param DO NOT MERGE: Reorder RecaptchaVerifier params so auth is the first param Jun 28, 2023
@hsubox76 hsubox76 changed the title DO NOT MERGE: Reorder RecaptchaVerifier params so auth is the first param Reorder RecaptchaVerifier params so auth is the first param Jul 5, 2023
@hsubox76 hsubox76 merged commit 1ff891c into master Jul 5, 2023
@hsubox76 hsubox76 deleted the chazzy-starter branch July 5, 2023 17:17
@google-oss-bot google-oss-bot mentioned this pull request Jul 6, 2023
Merged
@firebase firebase locked and limited conversation to collaborators Aug 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Xiaoshouzi-gh Xiaoshouzi-gh Xiaoshouzi-gh approved these changes

@prameshj prameshj prameshj approved these changes

@kevinthecheung kevinthecheung kevinthecheung approved these changes

@hsubox76 hsubox76 hsubox76 approved these changes

@egilmorez egilmorez Awaiting requested review from egilmorez

@markarndt markarndt Awaiting requested review from markarndt

@lisajian lisajian Awaiting requested review from lisajian

@renkelvin renkelvin Awaiting requested review from renkelvin

@sam-gc sam-gc Awaiting requested review from sam-gc

@dwyfrequency dwyfrequency Awaiting requested review from dwyfrequency

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v10
Development

Successfully merging this pull request may close these issues.
6 participants
@ch5zzy @google-oss-bot @prameshj @hsubox76 @kevinthecheung @Xiaoshouzi-gh