Skip to content
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

Merged
merged 1 commit into from
Apr 14, 2016
Merged

Conversation

taion
Copy link
Contributor

@taion taion commented Apr 14, 2016

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.

@taion
Copy link
Contributor Author

taion commented Apr 14, 2016

The main implication of this is that <IndexLink>s should be consistently quite a bit faster than normal <Link>s now, though I haven't benchmarked this at all.

// around the same quirks with basenames as in matchRoutes.
if (pathname.charAt(0) !== '/') {
pathname = `/${pathname}`
}
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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 :)

@ryanflorence ryanflorence added this to the next-2.3.0 milestone Apr 14, 2016
@taion
Copy link
Contributor Author

taion commented Apr 14, 2016

PR updated. I think this can actually go out in a patch release – there should be no change in observable functionality.

@timdorr
Copy link
Member

timdorr commented Apr 14, 2016

Agreed. Let's do a 2.2.3 with this in it.

@timdorr timdorr merged commit a25d9a6 into master Apr 14, 2016
@timdorr timdorr deleted the pathIsActive branch April 14, 2016 20:56
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants