-
Notifications
You must be signed in to change notification settings - Fork 137
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: using enum instead of strings for auth step types #1251
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
How to use the Graphite Merge QueueAdd the label graphite-merge-queue to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
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.
What's the motivation for this change? we use string consts in other places of the code base and actually use enums fairly sparingly
email_verify = "email_verify", | ||
otp_verify = "otp_verify", | ||
passkey_verify = "passkey_verify", | ||
passkey_create = "passkey_create", | ||
passkey_create_success = "passkey_create_success", | ||
email_completing = "email_completing", | ||
oauth_completing = "oauth_completing", | ||
initial = "initial", | ||
complete = "complete", | ||
eoa_connect = "eoa_connect", | ||
wallet_connect = "wallet_connect", | ||
pick_eoa = "pick_eoa", |
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.
nit: for the enum name, can we do capital case? ie EmailVerify
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.
change is done
@moldy530 my motivation for this PR is just that while working around the auth flow I felt the need to unify the source of the auth steps, I know it may be an opinionated change, but one of the advantages of using TS is that we can centralize these kind of things. In my personal experience, using strings (specially in big teams) gets to be really messy at some point and takes out several advantages of using TS with enums:
I know there are several other places where you're using strings instead of enums, I just wanted to propose making this change because it makes sense to me; this is not required or anything, it is just me thinking that enums in this particular case would be a better use |
Pull Request Checklist
yarn test
)site
folder, and guidelines for updating/adding docs can be found in the contribution guide)feat!: breaking change
)yarn lint:check
) and fix any issues? (yarn lint:write
)PR-Codex overview
This PR primarily focuses on refactoring the
authStep
handling in the authentication components by replacing string literals with anAuthStepType
enum for improved type safety and maintainability.Detailed summary
AuthStepType
enum to replace string literals insetAuthStep
calls.AuthStepType
for various authentication steps.authStep
types across the application.