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
Smarter stripPrefix #459
Changes from 4 commits
a110f6e
37f2ce8
22ca7e0
89b2a66
3a6d31e
c56bcbd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -26,7 +29,7 @@ export const parsePath = (path) => { | |
search = pathname.substr(searchIndex) | ||
pathname = pathname.substr(0, searchIndex) | ||
} | ||
|
||
pathname = decodeURI(pathname) | ||
|
||
return { | ||
|
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' | ||
|
@@ -168,4 +169,42 @@ describeHistory('a browser history', () => { | |
TestSequences.HashChangeTransitionHook(history, done) | ||
}) | ||
}) | ||
|
||
describe('basename', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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('/') | ||
}) | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I couldn't just default There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really hadn't thought about this before, but I just updated There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 That works! |
||
} | ||
|
||
const createKey = () => | ||
|
There was a problem hiding this comment.
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 beThere was a problem hiding this comment.
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, whichstartsWith
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.There was a problem hiding this comment.
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