Smarter stripPrefix #459
Smarter stripPrefix #459
Conversation
|
Thanks for the PR, @pshrmn. This looks great. Thanks to you I think I'm finally getting some words to use to describe to people what a basename is. "The slash-delimited portion of the pathname that is not used for routing". As for the tests, I think they'd be better if they used public API, yes. It does feel a little heavy, but tests that cover public API are the only ones that matter. |
|
We should also probably give the internal function a more descriptive name like |
|
Idea: we can have 2 internal functions: |
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.
|
Alright, so there is a potential bug here that needs to be addressed before this is merged: in let pathname = path || '/'However, if the path = '?test=ing'
pathname = '?test=ing'
// remove the search string
pathname = ''When that happens, should |
|
Yes, we should never have an empty pathname. The "empty" pathname is |
| (new RegExp('^' + prefix + '(\\/|\\?|#|$)', 'i')).test(path) | ||
|
|
||
| export const stripBasename = (path, prefix) => | ||
| hasBasename(path, prefix) ? path.substr(prefix.length) : path |
| @@ -168,4 +169,42 @@ describeHistory('a browser history', () => { | |||
| TestSequences.HashChangeTransitionHook(history, done) | |||
| }) | |||
| }) | |||
|
|
|||
| describe('basename', () => { | |||
There was a problem hiding this comment.
❤️ these tests. Very easy to read.
| 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('') |
There was a problem hiding this comment.
history.location.pathname should be / here.
| state, | ||
| key | ||
| } | ||
| return createLocation(path, state, key) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
This doesn't work if prefix has regex special symbols in them. Like $. It should be
export const hasBasename = (path, prefix) => path.startsWith(prefix)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks for the quick reply. I realised my mistake while makinging the fix. I created this PR for escaping special regex symbols: #544
Per remix-run/react-router#4866
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/prewill not be stripped). Being more selective about not stripping partial basename matches is potentially breaking, but I would consider it more a bug that you could do that before.@mjackson I believe that you have stated before that you don't like directly testing non-external APIs, but it felt like overkill to test these by triggering
getDOMLocationcalls when creating a browser/hash history.