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

fix ScopedHistory.createHref to prepend location with scoped history basePath #62407

Merged
merged 14 commits into from
Apr 11, 2020
Merged
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@

## ScopedHistory.createHref property

Creates an href (string) to the location.
Creates an href (string) to the location. If `prependBasePath` is true (default), it will prepend the location's path with the scoped history basePath.

<b>Signature:</b>

```typescript
createHref: (location: LocationDescriptorObject<HistoryLocationState>) => string;
createHref: (location: LocationDescriptorObject<HistoryLocationState>, { prependBasePath }?: {
prependBasePath?: boolean | undefined;
}) => string;
```
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export declare class ScopedHistory<HistoryLocationState = unknown> implements Hi
| --- | --- | --- | --- |
| [action](./kibana-plugin-core-public.scopedhistory.action.md) | | <code>Action</code> | The last action dispatched on the history stack. |
| [block](./kibana-plugin-core-public.scopedhistory.block.md) | | <code>(prompt?: string &#124; boolean &#124; History.TransitionPromptHook&lt;HistoryLocationState&gt; &#124; undefined) =&gt; UnregisterCallback</code> | Not supported. Use [AppMountParameters.onAppLeave](./kibana-plugin-core-public.appmountparameters.onappleave.md)<!-- -->. |
| [createHref](./kibana-plugin-core-public.scopedhistory.createhref.md) | | <code>(location: LocationDescriptorObject&lt;HistoryLocationState&gt;) =&gt; string</code> | Creates an href (string) to the location. |
| [createHref](./kibana-plugin-core-public.scopedhistory.createhref.md) | | <code>(location: LocationDescriptorObject&lt;HistoryLocationState&gt;, { prependBasePath }?: {</code><br/><code> prependBasePath?: boolean &#124; undefined;</code><br/><code> }) =&gt; string</code> | Creates an href (string) to the location. If <code>prependBasePath</code> is true (default), it will prepend the location's path with the scoped history basePath. |
| [createSubHistory](./kibana-plugin-core-public.scopedhistory.createsubhistory.md) | | <code>&lt;SubHistoryLocationState = unknown&gt;(basePath: string) =&gt; ScopedHistory&lt;SubHistoryLocationState&gt;</code> | Creates a <code>ScopedHistory</code> for a subpath of this <code>ScopedHistory</code>. Useful for applications that may have sub-apps that do not need access to the containing application's history. |
| [go](./kibana-plugin-core-public.scopedhistory.go.md) | | <code>(n: number) =&gt; void</code> | Send the user forward or backwards in the history stack. |
| [goBack](./kibana-plugin-core-public.scopedhistory.goback.md) | | <code>() =&gt; void</code> | Send the user one location back in the history stack. Equivalent to calling [ScopedHistory.go(-1)](./kibana-plugin-core-public.scopedhistory.go.md)<!-- -->. If no more entries are available backwards, this is a no-op. |
Expand Down
25 changes: 23 additions & 2 deletions src/core/public/application/scoped_history.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -268,11 +268,32 @@ 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`);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also expect:

expect(h.createHref({})).toEqual(`/app/wow`);

but looks like returns ''.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

prependBasePath was returning undefined when location.pathname was undefined. Changed it to return basepath instead in 460abe1

expect(h.createHref({})).toEqual(`/app/wow`);
expect(h.createHref({ pathname: '/new-page', search: '?alpha=true' })).toEqual(
`/new-page?alpha=true`
`/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`
);
Copy link
Contributor

@Dosant Dosant Apr 6, 2020

Choose a reason for hiding this comment

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

I started working on #58137 and used this pr.
Looks like in real world the basename would have a trailing slash. (at least this is the case for my example plugin)
so for this test it would be instead of

const h = new ScopedHistory(gh, '/app/wow');

it would be:

const h = new ScopedHistory(gh, '/app/wow/');

And then this test would fail with: /app/wow//new-page?alpha=true (2 slashes)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 ${this.basePath}/${path} always inserts a / between the paths.

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: '' }, { prependBasePath: false })).toEqual(`/`);
expect(
h.createHref({ pathname: '/new-page', search: '?alpha=true' }, { prependBasePath: false })
).toEqual(`/new-page?alpha=true`);
});
});

describe('action', () => {
Expand Down
20 changes: 17 additions & 3 deletions src/core/public/application/scoped_history.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = true }: { prependBasePath?: boolean } = {}
): 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);
};

Expand Down Expand Up @@ -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;
}

/**
Expand Down
4 changes: 3 additions & 1 deletion src/core/public/public.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -1196,7 +1196,9 @@ export class ScopedHistory<HistoryLocationState = unknown> implements History<Hi
constructor(parentHistory: History, basePath: string);
get action(): Action;
block: (prompt?: string | boolean | History.TransitionPromptHook<HistoryLocationState> | undefined) => UnregisterCallback;
createHref: (location: LocationDescriptorObject<HistoryLocationState>) => string;
createHref: (location: LocationDescriptorObject<HistoryLocationState>, { prependBasePath }?: {
prependBasePath?: boolean | undefined;
}) => string;
createSubHistory: <SubHistoryLocationState = unknown>(basePath: string) => ScopedHistory<SubHistoryLocationState>;
go: (n: number) => void;
goBack: () => void;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ const Nav = withRouter(({ history, navigateToApp }: NavProps) => (
{
id: 'home',
name: 'Home',
onClick: () => history.push('/'),
onClick: () => history.push(''),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

prependBasePathToString was incorrectly removing the trailing /, as this is fixed, I adapted the test plugins instead or changing every waitForUrlToBe('/app/foo'); to waitForUrlToBe('/app/foo/'); in test/plugin_functional/test_suites/core_plugins/applications.ts

'data-test-subj': 'fooNavHome',
},
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ const Nav = withRouter(({ history, navigateToApp }: NavProps) => (
{
id: 'home',
name: 'Home',
onClick: () => navigateToApp('bar', { path: '/' }),
onClick: () => navigateToApp('bar', { path: '' }),
'data-test-subj': 'barNavHome',
},
{
Expand Down