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

Smarter stripPrefix #459

Merged
merged 6 commits into from Apr 20, 2017
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions modules/LocationUtils.js
Expand Up @@ -42,6 +42,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
Expand Down
9 changes: 6 additions & 3 deletions modules/PathUtils.js
Expand Up @@ -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)
Copy link

Choose a reason for hiding this comment

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

This doesn't work if prefix has regex special symbols in them. Like $. It should be

export const hasBasename = (path, prefix) => path.startsWith(prefix)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A basename is meant to represent full segments, which startsWith cannot enforce on its own (the basename /test should not match the pathname /testing).

The prefix probably should be passed to a function that escapes any special regexp characters. I'm sure that a PR with tests would be appreciated for this.

Copy link

Choose a reason for hiding this comment

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

Thanks for the quick reply. I realised my mistake while makinging the fix. I created this PR for escaping special regex symbols: #544


export const stripBasename = (path, prefix) =>
hasBasename(path, prefix) ? path.substr(prefix.length) : path
Copy link
Member

Choose a reason for hiding this comment

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

👍


export const stripTrailingSlash = (path) =>
path.charAt(path.length - 1) === '/' ? path.slice(0, -1) : path
Expand All @@ -26,7 +29,7 @@ export const parsePath = (path) => {
search = pathname.substr(searchIndex)
pathname = pathname.substr(0, searchIndex)
}

pathname = decodeURI(pathname)

return {
Expand Down
39 changes: 39 additions & 0 deletions 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'
Expand Down Expand Up @@ -168,4 +169,42 @@ describeHistory('a browser history', () => {
TestSequences.HashChangeTransitionHook(history, done)
})
})

describe('basename', () => {
Copy link
Member

Choose a reason for hiding this comment

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

❤️ these tests. Very easy to read.

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('/')
})
})
})
40 changes: 40 additions & 0 deletions 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'
Expand Down Expand Up @@ -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('/')
})
})
})
4 changes: 2 additions & 2 deletions modules/__tests__/LocationUtils-test.js
Expand Up @@ -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'
})
Expand All @@ -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'
})
Expand Down
14 changes: 5 additions & 9 deletions modules/createBrowserHistory.js
Expand Up @@ -4,8 +4,8 @@ import { createLocation } from './LocationUtils'
import {
addLeadingSlash,
stripTrailingSlash,
stripPrefix,
parsePath,
hasBasename,
stripBasename,
createPath
} from './PathUtils'
import createTransitionManager from './createTransitionManager'
Expand Down Expand Up @@ -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)
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 couldn't just default pathname to / in parsePath when it was an empty string because that would break the resolving that is done using history.location I'm not altogether satisfied with this approach because now the hash history location objects will have a key property. Maybe this isn't a big deal, but it is a side effect of the change.

Copy link
Member

Choose a reason for hiding this comment

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

It's not a huge deal but I'd prefer it if hash history objects didn't have a key property. I think it would mislead people into thinking that we support key with hash history when we don't really.

It feels like this might be one of those changes that requires a more thorough refactoring. If that's the case, I'm happy to take it on. I've set aside some time today and tomorrow for OSS work.

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 really hadn't thought about this before, but I just updated createLocation to only set the key property when the key value is truthy.

Copy link
Member

Choose a reason for hiding this comment

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

👍 That works!

}

const createKey = () =>
Expand Down
10 changes: 5 additions & 5 deletions modules/createHashHistory.js
Expand Up @@ -5,8 +5,8 @@ import {
addLeadingSlash,
stripLeadingSlash,
stripTrailingSlash,
stripPrefix,
parsePath,
hasBasename,
stripBasename,
createPath
} from './PathUtils'
import createTransitionManager from './createTransitionManager'
Expand Down Expand Up @@ -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()
Expand Down