Smarter stripPrefix (#459)

* Smarter stripPrefix Does a case-insensitive match and will not strip the prefix if the prefix is a partial match (e.g., given the pathname "/prefix/pathname", the basename "/pre" will not be stripped. * Test public API, strip Basename * Use hasBasename * Have getDOMLocation call createLocation * Only add key property if key is provided * Test createLocation with no key
remix-run · Apr 20, 2017 · e075087 · e075087
1 parent fbf5f47
commit e075087
Show file tree

Hide file tree

Showing 7 changed files with 116 additions and 20 deletions.
diff --git a/modules/LocationUtils.js b/modules/LocationUtils.js
@@ -33,7 +33,8 @@ export const createLocation = (path, state, key, currentLocation) => {
       location.state = state
   }
 
-  location.key = key
+  if (key)
+    location.key = key
 
   if (currentLocation) {
     // Resolve incomplete/relative pathname relative to current location.
@@ -42,6 +43,11 @@ export const createLocation = (path, state, key, currentLocation) => {
     } else if (location.pathname.charAt(0) !== '/') {
       location.pathname = resolvePathname(location.pathname, currentLocation.pathname)
     }
+  } else {
+    // When there is no prior location and pathname is empty, set it to /
+    if (!location.pathname) {
+      location.pathname = '/'
+    }
   }
 
   return location

diff --git a/modules/PathUtils.js b/modules/PathUtils.js
@@ -4,8 +4,11 @@ export const addLeadingSlash = (path) =>
 export const stripLeadingSlash = (path) =>
   path.charAt(0) === '/' ? path.substr(1) : path
 
-export const stripPrefix = (path, prefix) =>
-  path.indexOf(prefix) === 0 ? path.substr(prefix.length) : path
+export const hasBasename = (path, prefix) => 
+  (new RegExp('^' + prefix + '(\\/|\\?|#|$)', 'i')).test(path)
+
+export const stripBasename = (path, prefix) =>
+  hasBasename(path, prefix) ? path.substr(prefix.length) : path
 
 export const stripTrailingSlash = (path) =>
   path.charAt(path.length - 1) === '/' ? path.slice(0, -1) : path
@@ -26,7 +29,7 @@ export const parsePath = (path) => {
     search = pathname.substr(searchIndex)
     pathname = pathname.substr(0, searchIndex)
   }
-
+  
   pathname = decodeURI(pathname)
 
   return {

diff --git a/modules/__tests__/BrowserHistory-test.js b/modules/__tests__/BrowserHistory-test.js
@@ -1,3 +1,4 @@
+import expect from 'expect'
 import createHistory from '../createBrowserHistory'
 import { canUseDOM, supportsHistory } from '../DOMUtils'
 import * as TestSequences from './TestSequences'
@@ -168,4 +169,42 @@ describeHistory('a browser history', () => {
       TestSequences.HashChangeTransitionHook(history, done)
     })
   })
+
+  describe('basename', () => {
+    it('strips the basename from the pathname', () => {
+      window.history.replaceState(null, null, '/prefix/pathname')
+      const history = createHistory({ basename: '/prefix' })
+      expect(history.location.pathname).toEqual('/pathname')
+    })
+
+    it('is not case-sensitive', () => {
+      window.history.replaceState(null, null, '/PREFIX/pathname')
+      const history = createHistory({ basename: '/prefix' })
+      expect(history.location.pathname).toEqual('/pathname')
+    })
+
+    it('does not strip partial prefix matches', () => {
+      window.history.replaceState(null, null, '/prefixed/pathname')
+      const history = createHistory({ basename: '/prefix' })
+      expect(history.location.pathname).toEqual('/prefixed/pathname')
+    })
+
+    it('strips when path is only the prefix', () => {
+      window.history.replaceState(null, null, '/prefix')
+      const history = createHistory({ basename: '/prefix' })
+      expect(history.location.pathname).toEqual('/')
+    })
+
+    it('strips with no pathname, but with a search string', () => {
+      window.history.replaceState(null, null, '/prefix?a=b')
+      const history = createHistory({ basename: '/prefix' })
+      expect(history.location.pathname).toEqual('/')
+    })
+
+    it('strips with no pathname, but with a hash string', () => {
+      window.history.replaceState(null, null, '/prefix#rest')
+      const history = createHistory({ basename: '/prefix' })
+      expect(history.location.pathname).toEqual('/')
+    })
+  })
 })
diff --git a/modules/__tests__/HashHistory-test.js b/modules/__tests__/HashHistory-test.js
@@ -1,3 +1,4 @@
+import expect from 'expect'
 import createHistory from '../createHashHistory'
 import { canUseDOM, supportsGoWithoutReloadUsingHash } from '../DOMUtils'
 import * as TestSequences from './TestSequences'
@@ -208,4 +209,43 @@ describeHistory('a hash history', () => {
       TestSequences.SlashHashPathCoding(history, done)
     })
   })
+
+
+  describe('basename', () => {
+    it('strips the basename from the pathname', () => {
+      window.location.hash = '#/prefix/pathname'
+      const history = createHistory({ basename: '/prefix' })
+      expect(history.location.pathname).toEqual('/pathname')
+    })
+
+    it('is not case-sensitive', () => {
+      window.location.hash = '#/PREFIX/pathname'
+      const history = createHistory({ basename: '/prefix' })
+      expect(history.location.pathname).toEqual('/pathname')
+    })
+
+    it('does not strip partial prefix matches', () => {
+      window.location.hash = '#/prefixed/pathname'
+      const history = createHistory({ basename: '/prefix' })
+      expect(history.location.pathname).toEqual('/prefixed/pathname')
+    })
+
+    it('strips when path is only the prefix', () => {
+      window.location.hash = '#/prefix'
+      const history = createHistory({ basename: '/prefix' })
+      expect(history.location.pathname).toEqual('/')
+    })
+
+    it('strips with no pathname, but with a search string', () => {
+      window.location.hash = '#/prefix?a=b'
+      const history = createHistory({ basename: '/prefix' })
+      expect(history.location.pathname).toEqual('/')
+    })
+
+    it('strips with no pathname, but with a hash string', () => {
+      window.location.hash = '#/prefix#rest'
+      const history = createHistory({ basename: '/prefix' })
+      expect(history.location.pathname).toEqual('/')
+    })
+  })
 })
diff --git a/modules/__tests__/LocationUtils-test.js b/modules/__tests__/LocationUtils-test.js
@@ -50,7 +50,7 @@ describe('createLocation', () => {
     describe('given as a string', () => {
       it('has the correct properties', () => {
         expect(createLocation('?the=query#the-hash')).toMatch({
-          pathname: '',
+          pathname: '/',
           search: '?the=query',
           hash: '#the-hash'
         })
@@ -60,7 +60,7 @@ describe('createLocation', () => {
     describe('given as an object', () => {
       it('has the correct properties', () => {
         expect(createLocation({ search: '?the=query', hash: '#the-hash' })).toMatch({
-          pathname: '',
+          pathname: '/',
           search: '?the=query',
           hash: '#the-hash'
         })
@@ -111,4 +111,16 @@ describe('createLocation', () => {
       })
     })
   })
+
+  describe('key', () => {
+    it('has a key property if a key is provided', () => {
+      const location = createLocation('/the/path', undefined, 'key')
+      expect(location).toIncludeKey('key')
+    })
+
+    it('has no key property if no key is provided', () => {
+      const location = createLocation('/the/path')
+      expect(location).toExcludeKey('key')
+    })
+  })
 })
diff --git a/modules/createBrowserHistory.js b/modules/createBrowserHistory.js
@@ -4,8 +4,8 @@ import { createLocation } from './LocationUtils'
 import {
   addLeadingSlash,
   stripTrailingSlash,
-  stripPrefix,
-  parsePath,
+  hasBasename,
+  stripBasename,
   createPath
 } from './PathUtils'
 import createTransitionManager from './createTransitionManager'
@@ -60,19 +60,15 @@ const createBrowserHistory = (props = {}) => {
     let path = pathname + search + hash
 
     warning(
-      !(basename && path.indexOf(basename) !== 0),
+      !(basename && hasBasename(path, basename)),
       'You are attempting to use a basename on a page whose URL path does not begin ' +
       'with the basename. Expected path "' + path + '" to begin with "' + basename + '".'
     )
 
     if (basename)
-      path = stripPrefix(path, basename)
+      path = stripBasename(path, basename)
 
-    return {
-      ...parsePath(path),
-      state,
-      key
-    }
+    return createLocation(path, state, key)
   }
 
   const createKey = () =>

diff --git a/modules/createHashHistory.js b/modules/createHashHistory.js
@@ -5,8 +5,8 @@ import {
   addLeadingSlash,
   stripLeadingSlash,
   stripTrailingSlash,
-  stripPrefix,
-  parsePath,
+  hasBasename,
+  stripBasename,
   createPath
 } from './PathUtils'
 import createTransitionManager from './createTransitionManager'
@@ -75,15 +75,15 @@ const createHashHistory = (props = {}) => {
     let path = decodePath(getHashPath())
 
     warning(
-      !(basename && path.indexOf(basename) !== 0),
+      !(basename && hasBasename(path, basename)),
       'You are attempting to use a basename on a page whose URL path does not begin ' +
       'with the basename. Expected path "' + path + '" to begin with "' + basename + '".'
     )
 
     if (basename)
-      path = stripPrefix(path, basename)
+      path = stripBasename(path, basename)
 
-    return parsePath(path)
+    return createLocation(path)
   }
 
   const transitionManager = createTransitionManager()