-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
🪟 🔧 remove cyclic dependencies with cloud routes #21424
Conversation
export enum CloudRoutes { | ||
Root = "/", | ||
AuthFlow = "/auth_flow", | ||
|
||
Metrics = "metrics", | ||
SelectWorkspace = "workspaces", | ||
Credits = "credits", | ||
|
||
// Auth routes | ||
Signup = "/signup", | ||
Login = "/login", | ||
ResetPassword = "/reset-password", | ||
|
||
// Firebase action routes | ||
// These URLs come from Firebase emails, and all have the same | ||
// action URL ("/verify-email") with different "mode" parameter | ||
// TODO: use a better action URL in Firebase email template | ||
FirebaseAction = "/verify-email", | ||
} |
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.
I can't think of a reason this needs to be an object as const
as opposed to an enum (and the OSS routePaths
is an enum so it seems right?)
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.
Code LGTM, tested locally for some routes and nothing seems to be broken by the change.
What
Removes cyclic dependency in our cloud routes to unblock part of #21421
How
Updates our cloud routes to use
cloudRoutePaths
in the same manner that OSS usesroutePaths
Recommended reading order
cloudRoutePaths.tsx