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

Allow to define and update a defaultPath for applications #64498

Merged
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
@@ -0,0 +1,13 @@
<!-- Do not edit this file. It is automatically generated by API Documenter. -->

[Home](./index.md) &gt; [kibana-plugin-core-public](./kibana-plugin-core-public.md) &gt; [AppBase](./kibana-plugin-core-public.appbase.md) &gt; [defaultPath](./kibana-plugin-core-public.appbase.defaultpath.md)

## AppBase.defaultPath property

Allow to define the default path a user should be directed to when navigating to the app. When defined, this value will be used as a default for the `path` option when calling [navigateToApp](./kibana-plugin-core-public.applicationstart.navigatetoapp.md)<!-- -->\`<!-- -->, and will also be appended to the [application navLink](./kibana-plugin-core-public.chromenavlink.md) in the navigation bar.

<b>Signature:</b>

```typescript
defaultPath?: string;
```
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ export interface AppBase
| [capabilities](./kibana-plugin-core-public.appbase.capabilities.md) | <code>Partial&lt;Capabilities&gt;</code> | Custom capabilities defined by the app. |
| [category](./kibana-plugin-core-public.appbase.category.md) | <code>AppCategory</code> | The category definition of the product See [AppCategory](./kibana-plugin-core-public.appcategory.md) See DEFAULT\_APP\_CATEGORIES for more reference |
| [chromeless](./kibana-plugin-core-public.appbase.chromeless.md) | <code>boolean</code> | Hide the UI chrome when the application is mounted. Defaults to <code>false</code>. Takes precedence over chrome service visibility settings. |
| [defaultPath](./kibana-plugin-core-public.appbase.defaultpath.md) | <code>string</code> | Allow to define the default path a user should be directed to when navigating to the app. When defined, this value will be used as a default for the <code>path</code> option when calling [navigateToApp](./kibana-plugin-core-public.applicationstart.navigatetoapp.md)<!-- -->\`<!-- -->, and will also be appended to the [application navLink](./kibana-plugin-core-public.chromenavlink.md) in the navigation bar. |
| [euiIconType](./kibana-plugin-core-public.appbase.euiicontype.md) | <code>string</code> | A EUI iconType that will be used for the app's icon. This icon takes precendence over the <code>icon</code> property. |
| [icon](./kibana-plugin-core-public.appbase.icon.md) | <code>string</code> | A URL to an image file used as an icon. Used as a fallback if <code>euiIconType</code> is not provided. |
| [id](./kibana-plugin-core-public.appbase.id.md) | <code>string</code> | The unique identifier of the application |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,5 @@ Defines the list of fields that can be updated via an [AppUpdater](./kibana-plug
<b>Signature:</b>

```typescript
export declare type AppUpdatableFields = Pick<AppBase, 'status' | 'navLinkStatus' | 'tooltip'>;
export declare type AppUpdatableFields = Pick<AppBase, 'status' | 'navLinkStatus' | 'tooltip' | 'defaultPath'>;
```
Original file line number Diff line number Diff line change
Expand Up @@ -29,5 +29,5 @@ export interface ChromeNavLink
| [subUrlBase](./kibana-plugin-core-public.chromenavlink.suburlbase.md) | <code>string</code> | A url base that legacy apps can set to match deep URLs to an application. |
| [title](./kibana-plugin-core-public.chromenavlink.title.md) | <code>string</code> | The title of the application. |
| [tooltip](./kibana-plugin-core-public.chromenavlink.tooltip.md) | <code>string</code> | A tooltip shown when hovering over an app link. |
| [url](./kibana-plugin-core-public.chromenavlink.url.md) | <code>string</code> | A url that legacy apps can set to deep link into their applications. |
| [url](./kibana-plugin-core-public.chromenavlink.url.md) | <code>string</code> | The route used to open the [default path](./kibana-plugin-core-public.appbase.defaultpath.md) of an application. If unset, <code>baseUrl</code> will be used instead. |

Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,7 @@

## ChromeNavLink.url property

> Warning: This API is now obsolete.
>
>
A url that legacy apps can set to deep link into their applications.
The route used to open the [default path](./kibana-plugin-core-public.appbase.defaultpath.md) of an application. If unset, `baseUrl` will be used instead.

<b>Signature:</b>

Expand Down
87 changes: 85 additions & 2 deletions src/core/public/application/application_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ describe('#setup()', () => {
).toThrowErrorMatchingInlineSnapshot(`"Applications cannot be registered after \\"setup\\""`);
});

it('allows to register a statusUpdater for the application', async () => {
it('allows to register an AppUpdater for the application', async () => {
const setup = service.setup(setupDeps);

const pluginId = Symbol('plugin');
Expand Down Expand Up @@ -118,6 +118,7 @@ describe('#setup()', () => {
updater$.next(app => ({
status: AppStatus.inaccessible,
tooltip: 'App inaccessible due to reason',
defaultPath: 'foo/bar',
}));

applications = await applications$.pipe(take(1)).toPromise();
Expand All @@ -128,6 +129,7 @@ describe('#setup()', () => {
legacy: false,
navLinkStatus: AppNavLinkStatus.default,
status: AppStatus.inaccessible,
defaultPath: 'foo/bar',
tooltip: 'App inaccessible due to reason',
})
);
Expand Down Expand Up @@ -209,7 +211,7 @@ describe('#setup()', () => {
});
});

describe('registerAppStatusUpdater', () => {
describe('registerAppUpdater', () => {
it('updates status fields', async () => {
const setup = service.setup(setupDeps);

Expand Down Expand Up @@ -413,6 +415,36 @@ describe('#setup()', () => {
})
);
});

it('allows to update the basePath', async () => {
const setup = service.setup(setupDeps);

const pluginId = Symbol('plugin');
setup.register(pluginId, createApp({ id: 'app1' }));

const updater = new BehaviorSubject<AppUpdater>(app => ({}));
setup.registerAppUpdater(updater);

const start = await service.start(startDeps);
await start.navigateToApp('app1');
expect(MockHistory.push).toHaveBeenCalledWith('/app/app1', undefined);
MockHistory.push.mockClear();

updater.next(app => ({ defaultPath: 'default-path' }));
Copy link
Contributor

Choose a reason for hiding this comment

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

from this example I'd expect it to be called as defaultSubpath / internalPath, etc.

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 the default part is important as the prop is used as a default in navigateToApp.
However in navigateToApp, the app path option is called path, not subPath, so I choose defaultPath.

But I'm fine with defaultSubPath if we think this is more explicit.

await start.navigateToApp('app1');
expect(MockHistory.push).toHaveBeenCalledWith('/app/app1/default-path', undefined);
MockHistory.push.mockClear();

updater.next(app => ({ defaultPath: 'another-path' }));
await start.navigateToApp('app1');
expect(MockHistory.push).toHaveBeenCalledWith('/app/app1/another-path', undefined);
MockHistory.push.mockClear();

updater.next(app => ({}));
await start.navigateToApp('app1');
expect(MockHistory.push).toHaveBeenCalledWith('/app/app1', undefined);
MockHistory.push.mockClear();
});
});

it("`registerMountContext` calls context container's registerContext", () => {
Expand Down Expand Up @@ -676,6 +708,57 @@ describe('#start()', () => {
expect(MockHistory.push).toHaveBeenCalledWith('/custom/path#/hash/router/path', undefined);
});

it('preserves trailing slash when path contains a hash', async () => {
const { register } = service.setup(setupDeps);

register(Symbol(), createApp({ id: 'app2', appRoute: '/custom/app-path' }));

const { navigateToApp } = await service.start(startDeps);
await navigateToApp('app2', { path: '#/' });
expect(MockHistory.push).toHaveBeenCalledWith('/custom/app-path#/', undefined);
MockHistory.push.mockClear();

await navigateToApp('app2', { path: '#/foo/bar/' });
expect(MockHistory.push).toHaveBeenCalledWith('/custom/app-path#/foo/bar/', undefined);
MockHistory.push.mockClear();

await navigateToApp('app2', { path: '/path#/' });
expect(MockHistory.push).toHaveBeenCalledWith('/custom/app-path/path#/', undefined);
MockHistory.push.mockClear();

await navigateToApp('app2', { path: '/path#/hash/' });
expect(MockHistory.push).toHaveBeenCalledWith('/custom/app-path/path#/hash/', undefined);
MockHistory.push.mockClear();

await navigateToApp('app2', { path: '/path/' });
expect(MockHistory.push).toHaveBeenCalledWith('/custom/app-path/path', undefined);
MockHistory.push.mockClear();
});

it('appends the defaultPath when the path parameter is not specified', async () => {
const { register } = service.setup(setupDeps);

register(Symbol(), createApp({ id: 'app1', defaultPath: 'default/path' }));
register(
Symbol(),
createApp({ id: 'app2', appRoute: '/custom-app-path', defaultPath: '/my-base' })
);

const { navigateToApp } = await service.start(startDeps);

await navigateToApp('app1', { path: 'defined-path' });
expect(MockHistory.push).toHaveBeenCalledWith('/app/app1/defined-path', undefined);

await navigateToApp('app1', {});
expect(MockHistory.push).toHaveBeenCalledWith('/app/app1/default/path', undefined);

await navigateToApp('app2', { path: 'defined-path' });
expect(MockHistory.push).toHaveBeenCalledWith('/custom-app-path/defined-path', undefined);

await navigateToApp('app2', {});
expect(MockHistory.push).toHaveBeenCalledWith('/custom-app-path/my-base', undefined);
});

it('includes state if specified', async () => {
const { register } = service.setup(setupDeps);

Expand Down
12 changes: 5 additions & 7 deletions src/core/public/application/application_service.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import {
Mounter,
} from './types';
import { getLeaveAction, isConfirmAction } from './application_leave';
import { appendAppPath } from './utils';

interface SetupDeps {
context: ContextSetup;
Expand Down Expand Up @@ -81,13 +82,7 @@ const getAppUrl = (mounters: Map<string, Mounter>, appId: string, path: string =
const appBasePath = mounters.get(appId)?.appRoute
? `/${mounters.get(appId)!.appRoute}`
: `/app/${appId}`;

// Only preppend slash if not a hash or query path
path = path.startsWith('#') || path.startsWith('?') ? path : `/${path}`;

return `${appBasePath}${path}`
.replace(/\/{2,}/g, '/') // Remove duplicate slashes
.replace(/\/$/, ''); // Remove trailing slash
return appendAppPath(appBasePath, path);
};

const allApplicationsFilter = '__ALL__';
Expand Down Expand Up @@ -290,6 +285,9 @@ export class ApplicationService {
},
navigateToApp: async (appId, { path, state }: { path?: string; state?: any } = {}) => {
if (await this.shouldNavigate(overlays)) {
if (path === undefined) {
path = applications$.value.get(appId)?.defaultPath;
}
this.appLeaveHandlers.delete(this.currentAppId$.value!);
this.navigate!(getAppUrl(availableMounters, appId, path), state);
this.currentAppId$.next(appId);
Expand Down
15 changes: 13 additions & 2 deletions src/core/public/application/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,13 @@ export interface AppBase {
*/
navLinkStatus?: AppNavLinkStatus;

/**
* Allow to define the default path a user should be directed to when navigating to the app.
* When defined, this value will be used as a default for the `path` option when calling {@link ApplicationStart.navigateToApp | navigateToApp}`,
* and will also be appended to the {@link ChromeNavLink | application navLink} in the navigation bar.
*/
defaultPath?: string;
Copy link
Contributor Author

@pgayvallet pgayvallet Apr 27, 2020

Choose a reason for hiding this comment

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

Note: this is on AppBase because AppUpdatableFields is a pick from this root class (and I didn't want to overcomplexify the AppUpdatableFields type with generics). However this property is only used for KP apps. This will be cleaned up 'naturally' once we drop legacy support

Copy link
Contributor

Choose a reason for hiding this comment

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

should we mark it as @deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not deprecated. It's just that even if on AppBase, it's effectively only usable by the KP App subclass, and not the LegacyApp one.


/**
* An {@link AppUpdater} observable that can be used to update the application {@link AppUpdatableFields} at runtime.
*
Expand Down Expand Up @@ -187,7 +194,10 @@ export enum AppNavLinkStatus {
* Defines the list of fields that can be updated via an {@link AppUpdater}.
* @public
*/
export type AppUpdatableFields = Pick<AppBase, 'status' | 'navLinkStatus' | 'tooltip'>;
export type AppUpdatableFields = Pick<
AppBase,
'status' | 'navLinkStatus' | 'tooltip' | 'defaultPath'
>;

/**
* Updater for applications.
Expand Down Expand Up @@ -642,7 +652,8 @@ export interface ApplicationStart {
* Navigate to a given app
*
* @param appId
* @param options.path - optional path inside application to deep link to
* @param options.path - optional path inside application to deep link to.
* If undefined, will use {@link AppBase.defaultPath | the app's default path}` as default.
* @param options.state - optional state to forward to the application
*/
navigateToApp(appId: string, options?: { path?: string; state?: any }): Promise<void>;
Expand Down
71 changes: 71 additions & 0 deletions src/core/public/application/utils.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { removeSlashes, appendAppPath } from './utils';

describe('removeSlashes', () => {
it('only removes duplicates by default', () => {
expect(removeSlashes('/some//url//to//')).toEqual('/some/url/to/');
expect(removeSlashes('some/////other//url')).toEqual('some/other/url');
});

it('remove trailing slash when `trailing` is true', () => {
expect(removeSlashes('/some//url//to//', { trailing: true })).toEqual('/some/url/to');
});

it('remove leading slash when `leading` is true', () => {
expect(removeSlashes('/some//url//to//', { leading: true })).toEqual('some/url/to/');
});

it('does not removes duplicates when `duplicates` is false', () => {
expect(removeSlashes('/some//url//to/', { leading: true, duplicates: false })).toEqual(
'some//url//to/'
);
expect(removeSlashes('/some//url//to/', { trailing: true, duplicates: false })).toEqual(
'/some//url//to'
);
});

it('accept mixed options', () => {
expect(
removeSlashes('/some//url//to/', { leading: true, duplicates: false, trailing: true })
).toEqual('some//url//to');
expect(
removeSlashes('/some//url//to/', { leading: true, duplicates: true, trailing: true })
).toEqual('some/url/to');
});
});

describe('appendAppPath', () => {
it('appends the appBasePath with given path', () => {
expect(appendAppPath('/app/my-app', '/some-path')).toEqual('/app/my-app/some-path');
expect(appendAppPath('/app/my-app/', 'some-path')).toEqual('/app/my-app/some-path');
expect(appendAppPath('/app/my-app', 'some-path')).toEqual('/app/my-app/some-path');
expect(appendAppPath('/app/my-app', '')).toEqual('/app/my-app');
});

it('preserves the trailing slash only if included in the hash', () => {
expect(appendAppPath('/app/my-app', '/some-path/')).toEqual('/app/my-app/some-path');
expect(appendAppPath('/app/my-app', '/some-path#/')).toEqual('/app/my-app/some-path#/');
expect(appendAppPath('/app/my-app', '/some-path#/hash/')).toEqual(
'/app/my-app/some-path#/hash/'
);
expect(appendAppPath('/app/my-app', '/some-path#/hash')).toEqual('/app/my-app/some-path#/hash');
});
});
54 changes: 54 additions & 0 deletions src/core/public/application/utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

/**
* Utility to remove trailing, leading or duplicate slashes.
* By default will only remove duplicates.
*/
export const removeSlashes = (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I move this to src/core/utils instead?

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 consider to move it when needed in another place

url: string,
{
trailing = false,
leading = false,
duplicates = true,
}: { trailing?: boolean; leading?: boolean; duplicates?: boolean } = {}
): string => {
if (duplicates) {
url = url.replace(/\/{2,}/g, '/');
}
if (trailing) {
url = url.replace(/\/$/, '');
}
if (leading) {
url = url.replace(/^\//, '');
}
return url;
};

export const appendAppPath = (appBasePath: string, path: string = '') => {
Copy link
Contributor

Choose a reason for hiding this comment

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

are there any tests for appendAppPath?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad, this was introduced in my last commit, and I did not add proper coverage for it. Will add.

// Only prepend slash if not a hash or query path
path = path === '' || path.startsWith('#') || path.startsWith('?') ? path : `/${path}`;
// Do not remove trailing slash when in hashbang
const removeTrailing = path.indexOf('#') === -1;
return removeSlashes(`${appBasePath}${path}`, {
trailing: removeTrailing,
duplicates: true,
leading: false,
});
};
Loading