-
-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Check for exact path match before checking routes #3313
Conversation
The main implication of this is that |
// around the same quirks with basenames as in matchRoutes. | ||
if (pathname.charAt(0) !== '/') { | ||
pathname = `/${pathname}` | ||
} |
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.
Since we're doing this twice, we can optimize a tiny bit by just moving this up to isActive
itself.
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.
Good point.
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.
In fact, given the similarity with currentPathname
as well, you can just do a method extraction here.
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 think it might be overkill to do that – this snippet is short/simple enough that I'm not too worried about duplicating it unnecessarily.
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, it's a nitpick. Do what you want :)
PR updated. I think this can actually go out in a patch release – there should be no change in observable functionality. |
Agreed. Let's do a 2.2.3 with this in it. |
Now that we've finally landed #3158 (hopefully for good), we can optimize
isActive
a bit.Paths are fully canonical now except for possibly the trailing slash.
Thus, for checking
indexOnly
active state, we only need to verify that the paths are the same, net of leading/trailing slash normalization. This ought to be a lot faster than actually running the path match.Additionally, this condition is also sufficient (but not necessary) for a route match in the general case.