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 2 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,10 @@

## 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?: boolean) => 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?: boolean) =&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
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
| Function | Description |
| --- | --- |
| [getDefaultSearchParams(config)](./kibana-plugin-plugins-data-server.getdefaultsearchparams.md) | |
| [getTotalLoaded({ total, failed, successful })](./kibana-plugin-plugins-data-server.gettotalloaded.md) | |
| [parseInterval(interval)](./kibana-plugin-plugins-data-server.parseinterval.md) | |
| [plugin(initializerContext)](./kibana-plugin-plugins-data-server.plugin.md) | Static code to be shared externally |
| [shouldReadFieldFromDocValues(aggregatable, esType)](./kibana-plugin-plugins-data-server.shouldreadfieldfromdocvalues.md) | |
Expand Down
12 changes: 11 additions & 1 deletion src/core/public/application/scoped_history.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -268,8 +268,18 @@ 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({ 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: '' }, false)).toEqual(`/`);
expect(h.createHref({ pathname: '/new-page', search: '?alpha=true' }, false)).toEqual(
`/new-page?alpha=true`
);
});
Expand Down
11 changes: 9 additions & 2 deletions src/core/public/application/scoped_history.ts
Original file line number Diff line number Diff line change
Expand Up @@ -219,12 +219,19 @@ 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
Copy link
Contributor

Choose a reason for hiding this comment

The 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
prependBasePath: boolean = true
{ prependBasePath = true }: { prependBasePath?: boolean } = {}

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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();
return this.parentHistory.createHref(location);
return this.parentHistory.createHref(
prependBasePath ? this.prependBasePath(location) : location
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that in some case, plugins might want to be able to use createHref to create href pointing outside of their basePath, so I added this optional parameter.

Being optional, it preserves the History interface

};

private prependBasePath(path: Path): Path;
Expand Down
2 changes: 1 addition & 1 deletion src/core/public/public.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -1195,7 +1195,7 @@ 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?: boolean) => string;
createSubHistory: <SubHistoryLocationState = unknown>(basePath: string) => ScopedHistory<SubHistoryLocationState>;
go: (n: number) => void;
goBack: () => void;
Expand Down