Support trailing slashes, not extraneous ones (#3285)

* Support trailing slashes, not extraneous ones * Fix unintentional regressions from removing extraneous slash support
remix-run · Apr 12, 2016 · ac0b761 · ac0b761
1 parent 4d66469
commit ac0b761
Show file tree

Hide file tree

Showing 7 changed files with 172 additions and 44 deletions.
diff --git a/modules/PatternUtils.js b/modules/PatternUtils.js
@@ -4,10 +4,6 @@ function escapeRegExp(string) {
   return string.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')
 }
 
-function escapeSource(string) {
-  return escapeRegExp(string).replace(/\/+/g, '/+')
-}
-
 function _compilePattern(pattern) {
   let regexpSource = ''
   const paramNames = []
@@ -17,7 +13,7 @@ function _compilePattern(pattern) {
   while ((match = matcher.exec(pattern))) {
     if (match.index !== lastIndex) {
       tokens.push(pattern.slice(lastIndex, match.index))
-      regexpSource += escapeSource(pattern.slice(lastIndex, match.index))
+      regexpSource += escapeRegExp(pattern.slice(lastIndex, match.index))
     }
 
     if (match[1]) {
@@ -42,7 +38,7 @@ function _compilePattern(pattern) {
 
   if (lastIndex !== pattern.length) {
     tokens.push(pattern.slice(lastIndex, pattern.length))
-    regexpSource += escapeSource(pattern.slice(lastIndex, pattern.length))
+    regexpSource += escapeRegExp(pattern.slice(lastIndex, pattern.length))
   }
 
   return {
@@ -82,17 +78,15 @@ export function compilePattern(pattern) {
  * - paramValues
  */
 export function matchPattern(pattern, pathname) {
-  // Make leading slashes consistent between pattern and pathname.
+  // Ensure pattern starts with leading slash for consistency with pathname.
   if (pattern.charAt(0) !== '/') {
     pattern = `/${pattern}`
   }
-  if (pathname.charAt(0) !== '/') {
-    pathname = `/${pathname}`
-  }
-
   let { regexpSource, paramNames, tokens } = compilePattern(pattern)
 
-  regexpSource += '/*' // Capture path separators
+  if (pattern.charAt(pattern.length - 1) !== '/') {
+    regexpSource += '/?' // Allow optional path separator at end.
+  }
 
   // Special-case patterns like '*' for catch-all routes.
   if (tokens[tokens.length - 1] === '*') {
@@ -106,18 +100,20 @@ export function matchPattern(pattern, pathname) {
     const matchedPath = match[0]
     remainingPathname = pathname.substr(matchedPath.length)
 
-    // If we didn't match the entire pathname, then make sure that the match we
-    // did get ends at a path separator (potentially the one we added above at
-    // the beginning of the path, if the actual match was empty).
-    if (
-      remainingPathname &&
-      matchedPath.charAt(matchedPath.length - 1) !== '/'
-    ) {
-      return {
-        remainingPathname: null,
-        paramNames,
-        paramValues: null
+    if (remainingPathname) {
+      // Require that the match ends at a path separator, if we didn't match
+      // the full path, so any remaining pathname is a new path segment.
+      if (matchedPath.charAt(matchedPath.length - 1) !== '/') {
+        return {
+          remainingPathname: null,
+          paramNames,
+          paramValues: null
+        }
       }
+
+      // If there is a remaining pathname, treat the path separator as part of
+      // the remaining pathname for properly continuing the match.
+      remainingPathname = `/${remainingPathname}`
     }
 
     paramValues = match.slice(1).map(v => v && decodeURIComponent(v))

diff --git a/modules/__tests__/_bc-isActive-test.js b/modules/__tests__/_bc-isActive-test.js
@@ -95,15 +95,15 @@ describe('v1 isActive', function () {
         })
       })
 
-      it('is active with extraneous slashes', function (done) {
+      it('is not active with extraneous slashes', function (done) {
         render((
           <Router history={createHistory('/parent/child')}>
             <Route path="/parent">
               <Route path="child" />
             </Route>
           </Router>
         ), node, function () {
-          expect(this.history.isActive('/parent////child////')).toBe(true)
+          expect(this.history.isActive('/parent////child////')).toBe(false)
           done()
         })
       })
@@ -293,7 +293,7 @@ describe('v1 isActive', function () {
         })
       })
 
-      it('is active with extraneous slashes', function (done) {
+      it('is not active with extraneous slashes', function (done) {
         render((
           <Router history={createHistory('/parent/child')}>
             <Route path="/parent">
@@ -303,8 +303,8 @@ describe('v1 isActive', function () {
             </Route>
           </Router>
         ), node, function () {
-          expect(this.history.isActive('/parent///child///', null)).toBe(true)
-          expect(this.history.isActive('/parent///child///', null, true)).toBe(true)
+          expect(this.history.isActive('/parent///child///', null)).toBe(false)
+          expect(this.history.isActive('/parent///child///', null, true)).toBe(false)
           done()
         })
       })
@@ -329,7 +329,7 @@ describe('v1 isActive', function () {
         })
       })
 
-      it('is active with extraneous slashes', function (done) {
+      it('is not active with extraneous slashes', function (done) {
         render((
           <Router history={createHistory('/parent/child')}>
             <Route path="/parent">
@@ -341,8 +341,8 @@ describe('v1 isActive', function () {
             </Route>
           </Router>
         ), node, function () {
-          expect(this.history.isActive('/parent///child///', null)).toBe(true)
-          expect(this.history.isActive('/parent///child///', null, true)).toBe(true)
+          expect(this.history.isActive('/parent///child///', null)).toBe(false)
+          expect(this.history.isActive('/parent///child///', null, true)).toBe(false)
           done()
         })
       })

diff --git a/modules/__tests__/isActive-test.js b/modules/__tests__/isActive-test.js
@@ -114,15 +114,43 @@ describe('isActive', function () {
         })
       })
 
-      it('is active with extraneous slashes', function (done) {
+      it('is active with trailing slash on pattern', function (done) {
         render((
           <Router history={createHistory('/parent/child')}>
             <Route path="/parent">
               <Route path="child" />
             </Route>
           </Router>
         ), node, function () {
-          expect(this.router.isActive('/parent////child////')).toBe(true)
+          expect(this.router.isActive('/parent/child/')).toBe(true)
+          done()
+        })
+      })
+
+      it('is active with trailing slash on location', function (done) {
+        render((
+          <Router history={createHistory('/parent/child/')}>
+            <Route path="/parent">
+              <Route path="child" />
+            </Route>
+          </Router>
+        ), node, function () {
+          expect(this.router.isActive('/parent/child')).toBe(true)
+          expect(this.router.isActive('/parent/child/')).toBe(true)
+          done()
+        })
+      })
+
+      it('is not active with extraneous slashes', function (done) {
+        render((
+          <Router history={createHistory('/parent/child')}>
+            <Route path="/parent">
+              <Route path="child" />
+            </Route>
+          </Router>
+        ), node, function () {
+          expect(this.router.isActive('/parent//child')).toBe(false)
+          expect(this.router.isActive('/parent/child//')).toBe(false)
           done()
         })
       })
@@ -336,7 +364,41 @@ describe('isActive', function () {
         })
       })
 
-      it('is active with extraneous slashes', function (done) {
+      it('is active with trailing slash on pattern', function (done) {
+        render((
+          <Router history={createHistory('/parent/child')}>
+            <Route path="/parent">
+              <Route path="child">
+                <IndexRoute />
+              </Route>
+            </Route>
+          </Router>
+        ), node, function () {
+          expect(this.router.isActive('/parent/child/')).toBe(true)
+          expect(this.router.isActive('/parent/child/', true)).toBe(true)
+          done()
+        })
+      })
+
+      it('is active with trailing slash on location', function (done) {
+        render((
+          <Router history={createHistory('/parent/child/')}>
+            <Route path="/parent">
+              <Route path="child">
+                <IndexRoute />
+              </Route>
+            </Route>
+          </Router>
+        ), node, function () {
+          expect(this.router.isActive('/parent/child')).toBe(true)
+          expect(this.router.isActive('/parent/child', true)).toBe(true)
+          expect(this.router.isActive('/parent/child/')).toBe(true)
+          expect(this.router.isActive('/parent/child/', true)).toBe(true)
+          done()
+        })
+      })
+
+      it('is not active with extraneous slashes', function (done) {
         render((
           <Router history={createHistory('/parent/child')}>
             <Route path="/parent">
@@ -346,8 +408,10 @@ describe('isActive', function () {
             </Route>
           </Router>
         ), node, function () {
-          expect(this.router.isActive('/parent///child///')).toBe(true)
-          expect(this.router.isActive('/parent///child///', true)).toBe(true)
+          expect(this.router.isActive('/parent//child')).toBe(false)
+          expect(this.router.isActive('/parent/child//')).toBe(false)
+          expect(this.router.isActive('/parent//child', true)).toBe(false)
+          expect(this.router.isActive('/parent/child//', true)).toBe(false)
           done()
         })
       })
@@ -372,7 +436,45 @@ describe('isActive', function () {
         })
       })
 
-      it('is active with extraneous slashes', function (done) {
+      it('is active with trailing slash on pattern', function (done) {
+        render((
+          <Router history={createHistory('/parent/child')}>
+            <Route path="/parent">
+              <Route path="child">
+                <Route>
+                  <IndexRoute />
+                </Route>
+              </Route>
+            </Route>
+          </Router>
+        ), node, function () {
+          expect(this.router.isActive('/parent/child/')).toBe(true)
+          expect(this.router.isActive('/parent/child/', true)).toBe(true)
+          done()
+        })
+      })
+
+      it('is active with trailing slash on location', function (done) {
+        render((
+          <Router history={createHistory('/parent/child/')}>
+            <Route path="/parent">
+              <Route path="child">
+                <Route>
+                  <IndexRoute />
+                </Route>
+              </Route>
+            </Route>
+          </Router>
+        ), node, function () {
+          expect(this.router.isActive('/parent/child')).toBe(true)
+          expect(this.router.isActive('/parent/child', true)).toBe(true)
+          expect(this.router.isActive('/parent/child/')).toBe(true)
+          expect(this.router.isActive('/parent/child/', true)).toBe(true)
+          done()
+        })
+      })
+
+      it('is not active with extraneous slashes', function (done) {
         render((
           <Router history={createHistory('/parent/child')}>
             <Route path="/parent">
@@ -384,8 +486,10 @@ describe('isActive', function () {
             </Route>
           </Router>
         ), node, function () {
-          expect(this.router.isActive('/parent///child///')).toBe(true)
-          expect(this.router.isActive('/parent///child///', true)).toBe(true)
+          expect(this.router.isActive('/parent//child')).toBe(false)
+          expect(this.router.isActive('/parent/child//')).toBe(false)
+          expect(this.router.isActive('/parent//child', true)).toBe(false)
+          expect(this.router.isActive('/parent/child//', true)).toBe(false)
           done()
         })
       })

diff --git a/modules/__tests__/matchPattern-test.js b/modules/__tests__/matchPattern-test.js
@@ -12,7 +12,7 @@ describe('matchPattern', function () {
   }
 
   it('works without params', function () {
-    assertMatch('/', '/path', 'path', [], [])
+    assertMatch('/', '/path', '/path', [], [])
   })
 
   it('works with named params', function () {

diff --git a/modules/__tests__/serverRendering-test.js b/modules/__tests__/serverRendering-test.js
@@ -162,6 +162,16 @@ describe('server rendering', function () {
     })
   })
 
+  it('supports basenames with trailing slash', function (done) {
+    match({ routes, location: '/dashboard', basename: '/nasebame/' }, function (error, redirectLocation, renderProps) {
+      const string = renderToString(
+        <RouterContext {...renderProps} />
+      )
+      expect(string).toMatch(/\/nasebame/)
+      done()
+    })
+  })
+
   describe('server/client consistency', function () {
     // Just render to static markup here to avoid having to normalize markup.
 

diff --git a/modules/isActive.js b/modules/isActive.js
@@ -81,6 +81,13 @@ function getMatchingRouteIndex(pathname, activeRoutes, activeParams) {
  * 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) {

diff --git a/modules/matchRoutes.js b/modules/matchRoutes.js
@@ -167,10 +167,23 @@ function matchRouteDeep(
  * Note: This operation may finish synchronously if no routes have an
  * asynchronous getChildRoutes method.
  */
-function matchRoutes(
+export default function matchRoutes(
   routes, location, callback,
-  remainingPathname=location.pathname, paramNames=[], paramValues=[]
+  remainingPathname, paramNames=[], paramValues=[]
 ) {
+  if (remainingPathname === undefined) {
+    // TODO: This is a little bit ugly, but it works around a quirk in history
+    // that strips the leading slash from pathnames when using basenames with
+    // trailing slashes.
+    if (location.pathname.charAt(0) !== '/') {
+      location = {
+        ...location,
+        pathname: `/${location.pathname}`
+      }
+    }
+    remainingPathname = location.pathname
+  }
+
   loopAsync(routes.length, function (index, next, done) {
     matchRouteDeep(
       routes[index], location, remainingPathname, paramNames, paramValues,
@@ -184,5 +197,3 @@ function matchRoutes(
     )
   }, callback)
 }
-
-export default matchRoutes