-
Notifications
You must be signed in to change notification settings - Fork 961
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
Support hashRoot in HashHistoryOptions #911
base: dev
Are you sure you want to change the base?
Support hashRoot in HashHistoryOptions #911
Conversation
As it stands now, I'm currently using this solution with a fork of UPDATE: |
Hey @thejohnhoffer, thanks for this! A couple of quick things:
Thanks! |
This comment has been minimized.
This comment has been minimized.
e8ef3ee
to
59580ad
Compare
657fc44
to
46bcb8e
Compare
46bcb8e
to
821d40c
Compare
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.
According to the tests, this is a non-breaking change with added support of hashRoot: ""
as a replacement for the lost feature of hashType: "noslash"
. For my purposes, this PR is now ready to merge.
@@ -21,12 +21,19 @@ import BlockPopWithoutListening from './TestSequences/BlockPopWithoutListening.j | |||
// const canGoWithoutReload = window.navigator.userAgent.indexOf('Firefox') === -1; | |||
// const describeGo = canGoWithoutReload ? describe : describe.skip; | |||
|
|||
describe('a hash history', () => { | |||
export const testHashHistory = (initialRoot, options) => { |
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 function can test both createHashHistory()
and createHashHistory({hashRoot: ""})
const { hashRoot = "/" } = options || {}; | ||
const historyHref = createPath(history.location); | ||
const windowHref = window.location.hash.substr(1); | ||
expect(historyHref.replace(/^\//, hashRoot)).toEqual(windowHref); |
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 solves issue #918
return input.match(/^\.\.\//) ? partial : { | ||
...partial, pathname: input.replace(base, root) | ||
}; |
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.
Interchange hashRoot
and '/'
(unless the pathname starts with "../")
This solves issue #912.
I'm happy to make any changes the maintainers decide should be made on this before merging. @chaance hopefully this PR is clearer to follow now, at only 35 lines of code (including tests). |
Hi @chaance -- I've again tested that the new and old tests all pass. My own alternative to this PR, use-hash-history, has a few users. I'm happy to continue maintaining my own package, but merging this PR would simplify the dependency tree of anyone who needs that feature. I'm happy to simplify, remove, or re-write some of the tests I've created for this PR. |
Adding a +1 to request that this PR gets merged, thanks for creating this, @thejohnhoffer! |
+1 for this PR |
Any chance to move this PR forward? |
@chaance how could we help moving this forward? |
@chaance "This branch has no conflicts with the base branch" - anything we could do? |
+1 for merging this PR |
Closes issue #912
Calling
createHashHistory
withhashRoot=""
replicates the losthashType="noslash"
fromhistory@4
. This PR is to address issue #8459 and issue #7703 of react-router.Update
Until this PR is merged, this is still possible with
react-router-dom@6.1.1
anduse-hash-history
.I accomplish this with a function to convert between
history.location
andwindow.location