From e3e05d98a0dedddc023bbe72bc015f2b1443e116 Mon Sep 17 00:00:00 2001
From: Jimmy Jia <tesrin@gmail.com>
Date: Thu, 14 Apr 2016 15:20:14 -0400
Subject: [PATCH] Check for exact path match before checking routes

---
 modules/__tests__/isActive-test.js |   6 ++
 modules/isActive.js                | 101 ++++++++++++++++-------------
 2 files changed, 61 insertions(+), 46 deletions(-)

diff --git a/modules/__tests__/isActive-test.js b/modules/__tests__/isActive-test.js
index d938ebe4b0..d71317ae19 100644
--- a/modules/__tests__/isActive-test.js
+++ b/modules/__tests__/isActive-test.js
@@ -114,6 +114,12 @@ describe('isActive', function () {
         })
       })
 
+      // This test case is a bit odd. A route with path /parent/child/ won't
+      // match /parent/child because of the trailing slash mismatch. However,
+      // this doesn't matter in practice, since it only comes up if your
+      // isActive pattern has a trailing slash but your route pattern doesn't,
+      // which would be an utterly bizarre thing to do.
+
       it('is active with trailing slash on pattern', function (done) {
         render((
           <Router history={createHistory('/parent/child')}>
diff --git a/modules/isActive.js b/modules/isActive.js
index b2f9e95f27..1c40a9c01c 100644
--- a/modules/isActive.js
+++ b/modules/isActive.js
@@ -38,18 +38,40 @@ function deepEqual(a, b) {
   return String(a) === String(b)
 }
 
-function paramsAreActive(paramNames, paramValues, activeParams) {
-  // FIXME: This doesn't work on repeated params in activeParams.
-  return paramNames.every(function (paramName, index) {
-    return String(paramValues[index]) === String(activeParams[paramName])
-  })
+/**
+ * Returns true if the current pathname matches the supplied one, net of
+ * leading and trailing slash normalization. This is sufficient for an
+ * indexOnly route match.
+ */
+function pathIsActive(pathname, currentPathname) {
+  // Normalize leading slash for consistency. Leading slash on pathname has
+  // already been normalized in isActive. See caveat there.
+  if (currentPathname.charAt(0) !== '/') {
+    currentPathname = `/${currentPathname}`
+  }
+
+  // Normalize the end of both path names too. Maybe `/foo/` shouldn't show
+  // `/foo` as active, but in this case, we would already have failed the
+  // match.
+  if (pathname.charAt(pathname.length - 1) !== '/') {
+    pathname += '/'
+  }
+  if (currentPathname.charAt(currentPathname.length - 1) !== '/') {
+    currentPathname += '/'
+  }
+
+  return currentPathname === pathname
 }
 
-function getMatchingRouteIndex(pathname, activeRoutes, activeParams) {
+/**
+ * Returns true if the given pathname matches the active routes and params.
+ */
+function routeIsActive(pathname, routes, params) {
   let remainingPathname = pathname, paramNames = [], paramValues = []
 
-  for (let i = 0, len = activeRoutes.length; i < len; ++i) {
-    const route = activeRoutes[i]
+  // for...of would work here but it's probably slower post-transpilation.
+  for (let i = 0, len = routes.length; i < len; ++i) {
+    const route = routes[i]
     const pattern = route.path || ''
 
     if (pattern.charAt(0) === '/') {
@@ -58,49 +80,24 @@ function getMatchingRouteIndex(pathname, activeRoutes, activeParams) {
       paramValues = []
     }
 
-    if (remainingPathname !== null) {
+    if (remainingPathname !== null && pattern) {
       const matched = matchPattern(pattern, remainingPathname)
       remainingPathname = matched.remainingPathname
       paramNames = [ ...paramNames, ...matched.paramNames ]
       paramValues = [ ...paramValues, ...matched.paramValues ]
-    }
 
-    if (
-      remainingPathname === '' &&
-      route.path &&
-      paramsAreActive(paramNames, paramValues, activeParams)
-    )
-      return i
-  }
-
-  return null
-}
-
-/**
- * Returns true if the given pathname matches the active routes
- * and params.
- */
-function routeIsActive(pathname, routes, params, indexOnly) {
-  // TODO: This is a bit ugly. It keeps around support for treating pathnames
-  // without preceding slashes as absolute paths, but possibly also works
-  // around the same quirks with basenames as in matchRoutes.
-  if (pathname.charAt(0) !== '/') {
-    pathname = `/${pathname}`
-  }
-
-  const i = getMatchingRouteIndex(pathname, routes, params)
-
-  if (i === null) {
-    // No match.
-    return false
-  } else if (!indexOnly) {
-    // Any match is good enough.
-    return true
+      if (remainingPathname === '') {
+        // We have an exact match on the route. Just check that all the params
+        // match.
+        // FIXME: This doesn't work on repeated params.
+        return paramNames.every((paramName, index) => (
+          String(paramValues[index]) === String(params[paramName])
+        ))
+      }
+    }
   }
 
-  // If any remaining routes past the match index have paths, then we can't
-  // be on the index route.
-  return routes.slice(i + 1).every(route => !route.path)
+  return false
 }
 
 /**
@@ -127,8 +124,20 @@ export default function isActive(
   if (currentLocation == null)
     return false
 
-  if (!routeIsActive(pathname, routes, params, indexOnly))
-    return false
+  // TODO: This is a bit ugly. It keeps around support for treating pathnames
+  // without preceding slashes as absolute paths, but possibly also works
+  // around the same quirks with basenames as in matchRoutes.
+  if (pathname.charAt(0) !== '/') {
+    pathname = `/${pathname}`
+  }
+
+  if (!pathIsActive(pathname, currentLocation.pathname)) {
+    // The path check is necessary and sufficient for indexOnly, but otherwise
+    // we still need to check the routes.
+    if (indexOnly || !routeIsActive(pathname, routes, params)) {
+      return false
+    }
+  }
 
   return queryIsActive(query, currentLocation.query)
 }