-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
fix ScopedHistory.createHref to prepend location with scoped history basePath #62407
Changes from 11 commits
dc8dc7f
460ec47
460abe1
b6c2107
abd75e1
3762fde
ab763a3
7604932
eff1faf
e4b8eeb
737c5df
052cdea
353277d
1fb7fc6
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 |
---|---|---|
|
@@ -268,8 +268,29 @@ describe('ScopedHistory', () => { | |
const gh = createMemoryHistory(); | ||
gh.push('/app/wow'); | ||
const h = new ScopedHistory(gh, '/app/wow'); | ||
expect(h.createHref({ pathname: '' })).toEqual(`/`); | ||
expect(h.createHref({ pathname: '' })).toEqual(`/app/wow`); | ||
expect(h.createHref({})).toEqual(`/app/wow`); | ||
expect(h.createHref({ pathname: '/new-page', search: '?alpha=true' })).toEqual( | ||
`/app/wow/new-page?alpha=true` | ||
); | ||
}); | ||
|
||
it('behave correctly with slash-ending basePath', () => { | ||
const gh = createMemoryHistory(); | ||
gh.push('/app/wow/'); | ||
const h = new ScopedHistory(gh, '/app/wow/'); | ||
expect(h.createHref({ pathname: '' })).toEqual(`/app/wow/`); | ||
expect(h.createHref({ pathname: '/new-page', search: '?alpha=true' })).toEqual( | ||
`/app/wow/new-page?alpha=true` | ||
); | ||
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 started working on #58137 and used this pr.
it would be:
And then this test would fail with: 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. You are right. This was caused by private prependBasePathToString(path: string): string {
path = path.startsWith('/') ? path.slice(1) : path;
return path.length ? `${this.basePath}/${path}` : this.basePath;
} Where Fixed in 460abe1 |
||
}); | ||
|
||
it('skips the scoped history path when `prependBasePath` is false', () => { | ||
const gh = createMemoryHistory(); | ||
gh.push('/app/wow'); | ||
const h = new ScopedHistory(gh, '/app/wow'); | ||
expect(h.createHref({ pathname: '' }, false)).toEqual(`/`); | ||
expect(h.createHref({ pathname: '/new-page', search: '?alpha=true' }, false)).toEqual( | ||
`/new-page?alpha=true` | ||
); | ||
}); | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -219,11 +219,26 @@ export class ScopedHistory<HistoryLocationState = unknown> | |||||
|
||||||
/** | ||||||
* Creates an href (string) to the location. | ||||||
* If `prependBasePath` is true (default), it will prepend the location's path with the scoped history basePath. | ||||||
* | ||||||
* @param location | ||||||
* @param prependBasePath | ||||||
*/ | ||||||
public createHref = (location: LocationDescriptorObject<HistoryLocationState>): Href => { | ||||||
public createHref = ( | ||||||
location: LocationDescriptorObject<HistoryLocationState>, | ||||||
prependBasePath: boolean = true | ||||||
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. nit: could we use an options object for this to make adding things in the future non-breaking?
Suggested change
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. 👍 Don't even know why I didn't do it that way in the first place. Will do. |
||||||
): Href => { | ||||||
this.verifyActive(); | ||||||
if (prependBasePath) { | ||||||
location = this.prependBasePath(location); | ||||||
if (location.pathname === undefined) { | ||||||
// we always want to create an url relative to the basePath | ||||||
// so if pathname is not present, we use the history's basePath as default | ||||||
// we are doing that here because `prependBasePath` should not | ||||||
// alter pathname for other method calls | ||||||
location.pathname = this.basePath; | ||||||
} | ||||||
} | ||||||
return this.parentHistory.createHref(location); | ||||||
}; | ||||||
|
||||||
|
@@ -254,8 +269,7 @@ export class ScopedHistory<HistoryLocationState = unknown> | |||||
* Prepends the base path to string. | ||||||
*/ | ||||||
private prependBasePathToString(path: string): string { | ||||||
path = path.startsWith('/') ? path.slice(1) : path; | ||||||
return path.length ? `${this.basePath}/${path}` : this.basePath; | ||||||
return path.length ? `${this.basePath}/${path}`.replace(/\/{2,}/g, '/') : this.basePath; | ||||||
} | ||||||
|
||||||
/** | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -95,7 +95,7 @@ const Nav = withRouter(({ history, navigateToApp }: NavProps) => ( | |
{ | ||
id: 'home', | ||
name: 'Home', | ||
onClick: () => history.push('/'), | ||
onClick: () => history.push(''), | ||
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.
|
||
'data-test-subj': 'fooNavHome', | ||
}, | ||
{ | ||
|
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.
I'd also expect:
but looks like returns
''
.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.
prependBasePath
was returningundefined
whenlocation.pathname
was undefined. Changed it to returnbasepath
instead in 460abe1