-
Notifications
You must be signed in to change notification settings - Fork 78
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
Ensure to match against the most specific route #87
Conversation
src/routing/Router.ts
Outdated
onExit | ||
}; | ||
if (defaultRoute) { | ||
this._defaultOutlet = outlet; | ||
} | ||
for (let i = 0; i < segments.length; i++) { | ||
const segment = segments[i]; | ||
if (typeof segment === 'string' && segment[0] === '{') { | ||
route.score += 7; |
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.
Might want to create constants for these numbers so we know what they mean!
@@ -24,6 +25,9 @@ export interface OutletEvent extends EventObject<string> { | |||
action: 'enter' | 'exit'; | |||
} | |||
|
|||
const ROUTE_SEGMENT_SCORE = 7; |
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.
These numbers look fairly arbitrary at first glance. Perhaps a comment to clarify?
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.
They actually are fairly arbitrary, I went for a score that meant we could introduce other types of segments in the future (like maybe wild card, which would have an even smaller score).
Type: feature/bug
The following has been addressed in the PR:
prettier
as per the readme code style guidelinesDescription:
For routing the first match route specified in the configuration will be considered "the one", even if there is a more exact match in a later route within the configuration.
Assuming a routing configuration:
Current Behaviour
Routing to
/bar
, should match against thebar
outlet as it's more specific (a string literal match) but currently would match against theparam
outlet because it is registered first.Routing to
/foo/baz
would currently match against the outlet,foo-one
with a type of partial, rather then correctly matching against the more exact outlet matchbaz
.Updated Bahaviour
Routing to
/bar
, will match against thebar
outlet as it's more specific (a string literal match).Routing to
/foo/baz
will match against outletbaz
.Resolves #83