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 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions modules/PathUtils.js
Original file line number Diff line number Diff line change
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 Down
39 changes: 39 additions & 0 deletions modules/__tests__/BrowserHistory-test.js
Original file line number Diff line number Diff line change
@@ -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('')
Copy link
Member

Choose a reason for hiding this comment

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

history.location.pathname should be / here.

})
})
})
42 changes: 42 additions & 0 deletions modules/__tests__/HashHistory-test.js
Original file line number Diff line number Diff line change
@@ -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,45 @@ 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', () => {
console.log('\n\n\n\n\n\n\n\n')
window.location.hash = '#/prefix#rest'
const history = createHistory({ basename: '/prefix' })
expect(history.location.pathname).toEqual('')
console.log('\n\n\n\n\n\n\n\n')
})
})
})
7 changes: 4 additions & 3 deletions modules/createBrowserHistory.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ import { createLocation } from './LocationUtils'
import {
addLeadingSlash,
stripTrailingSlash,
stripPrefix,
hasBasename,
stripBasename,
parsePath,
createPath
} from './PathUtils'
Expand Down Expand Up @@ -60,13 +61,13 @@ 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),
Expand Down
7 changes: 4 additions & 3 deletions modules/createHashHistory.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ import {
addLeadingSlash,
stripLeadingSlash,
stripTrailingSlash,
stripPrefix,
hasBasename,
stripBasename,
parsePath,
createPath
} from './PathUtils'
Expand Down Expand Up @@ -75,13 +76,13 @@ 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)
}
Expand Down