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
feat: support arbitrary login redirect routes #522
feat: support arbitrary login redirect routes #522
Conversation
This is requested for EE |
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.
Looks good! Just minor comments, especially around avoiding mutation on function parameters as it can affect reactivity when dealing with non-primitive variables.
webui/react/src/utils/routes.ts
Outdated
|
||
export const parseUrl = (url: string): URL => { | ||
if (!isFullPath(url)) { | ||
if (!url.startsWith('/')) url = '/' + url; // TODO assume url is absolute, or we could throw |
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.
suggestion: could be a useful function
export const isAbsolutePath = (url: string): boolean => url.startsWith('/');
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.
thoughts about handling relative urls? assume they're absolute or throw
webui/react/src/utils/routes.ts
Outdated
|
||
export const parseUrl = (url: string): URL => { | ||
if (!isFullPath(url)) { | ||
if (!url.startsWith('/')) url = '/' + url; // TODO assume url is absolute, or we could throw |
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.
suggestion: avoid overwriting/mutating function parameters.
For this particular case it's not as significant because it's just a string, but would be good practice for when the objects are complex.
We should add this eslint rule: https://eslint.org/docs/rules/no-param-reassign
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.
sounds good. and the eslint rule sounds good too and if we need to do that for maybe performance reasons we can ignore it case by case
1f338b9
to
5980a5c
Compare
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.
LGTM!
Description
DET-3068
Test Plan
Commentary (optional)