-
Notifications
You must be signed in to change notification settings - Fork 1.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
[REF-2643]Throw Errors for duplicate Routes #3155
Conversation
Needs reflex-dev/reflex-web#659 to pass CI |
reflex/app.py
Outdated
raise ValueError( | ||
f"You cannot define a route with the same specificity as a optional catch-all route ('{route}' and '{new_route}')" | ||
) | ||
def replace_brackets_with_keywords(input_string): |
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.
We can move this outside of this function, so it won't get redefined every time
reflex/app.py
Outdated
# / posts/[[...slug2]]-> /posts/__DOUBLE_CATCHALL_SEGMENT__ | ||
# /posts/[...slug3]-> /posts/__SINGLE_CATCHALL_SEGMENT__ | ||
|
||
# Replace [[...<slug>]] with __DOUBLE_CATCHALL_SEGMENT__ |
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 wonder if as a short-circuit we can first check if "[" in input_string
and only do the regexes if necessary.
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.
Yeah, i actually had that logic right below this function which checks for the presence of a replaced keyword, but I can move the logic up so the replace_brackets_with_keyword
function is not called for non dynamic routes
constants.RouteRegex.SINGLE_CATCHALL_SEGMENT, | ||
constants.RouteRegex.DOUBLE_CATCHALL_SEGMENT, | ||
) | ||
for route in self.pages: |
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.
This will end up doing n^2 comparisons - probably alright, but maybe we should save the hash value along with it when we add page?
With this PR an error will be thrown when two pages have the same route.
Eg.
Note that
add_page
takes precedence over@rx.page
NB: This PR does not handle duplicates for dynamic routes as that is a much more complex use case. Will make a separate PR as a follow-up to handle that case.