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.

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

fix: improve mfa.enroll return types #956

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

aloisklink
Copy link
Contributor

What kind of change does this PR introduce?

Bug fix for the TypeScript types.

What is the current behavior?

What is the new behavior?

Use TypeScript's function overloading to return

  • TOTP data when mfa.enroll was passed factorType: 'totp',
  • phone data when mfa.enroll was passed factorType: 'phone'.

This fixes a breaking change in the types in b957c30.

Additional context

This PR would also expose the new following types:

  • MFAEnrollTOTPParams
  • MFAEnrollPhoneParams
  • AuthMFAEnrollTOTPResponse
  • AuthMFAEnrollPhoneResponse
  • AuthMFAEnrollErrorResponse (it might be better to roll this type in with the other two).

It might be better to keep these types private, since once they're exposed, you can't easily remove them again without creating a breaking change.

Use TypeScript's operator overloading to return
- TOTP data when `mfa.enroll` was passed `factorType: 'totp'`,
- phone data when `mfa.enroll` was passed `factorType: 'phone'`.

This fixes a breaking change in the types in
b957c30.
@J0
Copy link
Contributor

J0 commented Sep 17, 2024

It might be better to keep these types private, since once they're exposed, you can't easily remove them again without creating a breaking change.

Would agree with this

Thanks for the PR! Looks great so far

@J0 J0 merged commit 8a1ec06 into supabase:master Sep 17, 2024
3 checks passed
@J0
Copy link
Contributor

J0 commented Sep 17, 2024

Will follow up with the corresponding PR for challengeParams and verifyParams (if they are at all needed)

Upd: shouldn't be needed, except in relation to MFA (WebAuthn).

@aloisklink
Copy link
Contributor Author

It might be better to keep these types private, since once they're exposed, you can't easily remove them again without creating a breaking change.

Would agree with this

If you are still interested in this, I think the easiest way to do this would be to make a new file called something like src/lib/internal-types.ts that isn't exported outside of the package using export * from './lib/types'.

Then, src/GoTrueClient.ts and src/lib/types.ts could both import types from this file.

(the hardest thing would be picking the right conventional commit prefix 😆, since this PR has already been merged, but not released, so it's not yet a breaking change)

Will follow up with the corresponding PR for challengeParams and verifyParams (if they are at all needed)

Upd: shouldn't be needed, except in relation to MFA (WebAuthn).

Yep, we use both supabase.auth.mfa.challenge and supabase.auth.mfa.verify in our code (based of the example in https://github.com/supabase/supabase/blob/10ba4999cd33f796e89c50a0821e46b3d346eb7c/apps/docs/content/guides/auth/auth-mfa/totp.mdx?plain=1#L245-L261), and v2.65.0 works fine in those cases!

And I'm excited to see WebAuthn support!

@aloisklink aloisklink deleted the fix/fix-mfa.enroll-return-types branch September 18, 2024 06:55
@J0
Copy link
Contributor

J0 commented Sep 23, 2024

@aloisklink, thanks for the update.

I think an internal-types file would work. Let me do that shortly. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mfa.enroll's return type has a TypeScript breaking change in v2.65.0
2 participants