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.

Sign up for GitHub

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

Jump to bottom

Remove react references from core Notifications apis #49573

Merged
merged 25 commits into from
Nov 14, 2019
Merged

Remove react references from core Notifications apis #49573

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
688c5a2
add reactMount util to kibana_react
pgayvallet Oct 28, 2019
678eb13
add MountPoint types and utility
pgayvallet Oct 28, 2019
6b986ee
adapt toast API to no longer accept react elements
pgayvallet Oct 28, 2019
5464cb2
adapt calls by using reactMount
pgayvallet Oct 28, 2019
ee61086
update generated doc
pgayvallet Oct 28, 2019
bd036bc
add custom snapshot serializer for reactMount
pgayvallet Oct 29, 2019
a49de40
fix unit tests
pgayvallet Oct 29, 2019
71f530b
adapt non-ts calls
pgayvallet Oct 29, 2019
b6f5d15
do not add __reactMount__ property in production
pgayvallet Oct 31, 2019
bb779fb
remove string check on createNotifications
pgayvallet Oct 31, 2019
c41f2db
fix typo and small fix using obj spread
pgayvallet Oct 31, 2019
ce1d179
improve react mount snapshot serializer
pgayvallet Oct 31, 2019
78d45af
simplify convertToEui
pgayvallet Oct 31, 2019
7de4066
rename reactMount to toMountPoint
pgayvallet Oct 31, 2019
0f391b2
Merge remote-tracking branch 'upstream/master' into kbn-36425-agnosti…
pgayvallet Oct 31, 2019
8e18eb0
adapt newly added calls
pgayvallet Oct 31, 2019
ab81138
move mount types to proper file
pgayvallet Nov 1, 2019
081f878
use new Mount types for OverlayBanner apis
pgayvallet Nov 1, 2019
7fa9e5d
fixing typo
pgayvallet Nov 11, 2019
53d8da5
Merge remote-tracking branch 'upstream/master' into kbn-36425-agnosti…
pgayvallet Nov 11, 2019
dad7ee6
Merge remote-tracking branch 'upstream/master' into kbn-36425-agnosti…
pgayvallet Nov 12, 2019
b944611
Merge remote-tracking branch 'upstream/master' into kbn-36425-agnosti…
pgayvallet Nov 12, 2019
5aede50
adapt new calls
pgayvallet Nov 12, 2019
c869fea
Merge remote-tracking branch 'upstream/master' into kbn-36425-agnosti…
pgayvallet Nov 14, 2019
912124c
use destructured imports
pgayvallet Nov 14, 2019
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
253 changes: 128 additions & 125 deletions docs/development/core/public/kibana-plugin-public.md
View file
Open in desktop

Large diffs are not rendered by default.

13 changes: 13 additions & 0 deletions docs/development/core/public/kibana-plugin-public.mountpoint.md
View file
Open in desktop
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-public](./kibana-plugin-public.md) &gt; [MountPoint](./kibana-plugin-public.mountpoint.md)

## MountPoint type

A function that will mount the banner inside the provided element.

<b>Signature:</b>

```typescript
export declare type MountPoint = (element: HTMLElement) => UnmountCallback;
```
13 changes: 13 additions & 0 deletions docs/development/core/public/kibana-plugin-public.toast.md
View file
Open in desktop
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-public](./kibana-plugin-public.md) &gt; [Toast](./kibana-plugin-public.toast.md)

## Toast type

<b>Signature:</b>

```typescript
export declare type Toast = ToastInputFields & {
id: string;
};
```
2 changes: 1 addition & 1 deletion docs/development/core/public/kibana-plugin-public.toastinput.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,5 @@ Inputs for [IToasts](./kibana-plugin-public.itoasts.md) APIs.
<b>Signature:</b>

```typescript
export declare type ToastInput = string | ToastInputFields | Promise<ToastInputFields>;
export declare type ToastInput = string | ToastInputFields;
```
5 changes: 4 additions & 1 deletion docs/development/core/public/kibana-plugin-public.toastinputfields.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@ Allowed fields for [ToastInput](./kibana-plugin-public.toastinput.md)<!-- -->.
<b>Signature:</b>

```typescript
export declare type ToastInputFields = Pick<Toast, Exclude<keyof Toast, 'id'>>;
export declare type ToastInputFields = Pick<EuiToast, Exclude<keyof EuiToast, 'id' | 'text' | 'title'>> & {
title?: string | MountPoint;
text?: string | MountPoint;
};
```

## Remarks
Expand Down
2 changes: 1 addition & 1 deletion docs/development/core/public/kibana-plugin-public.toastsapi.add.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,5 @@ add(toastOrTitle: ToastInput): Toast;

`Toast`

a
a [Toast](./kibana-plugin-public.toast.md)

2 changes: 1 addition & 1 deletion docs/development/core/public/kibana-plugin-public.toastsapi.adddanger.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,5 @@ addDanger(toastOrTitle: ToastInput): Toast;

`Toast`

a
a [Toast](./kibana-plugin-public.toast.md)

2 changes: 1 addition & 1 deletion docs/development/core/public/kibana-plugin-public.toastsapi.adderror.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,5 @@ addError(error: Error, options: ErrorToastOptions): Toast;

`Toast`

a
a [Toast](./kibana-plugin-public.toast.md)

2 changes: 1 addition & 1 deletion docs/development/core/public/kibana-plugin-public.toastsapi.addsuccess.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,5 @@ addSuccess(toastOrTitle: ToastInput): Toast;

`Toast`

a
a [Toast](./kibana-plugin-public.toast.md)

2 changes: 1 addition & 1 deletion docs/development/core/public/kibana-plugin-public.toastsapi.addwarning.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,5 @@ addWarning(toastOrTitle: ToastInput): Toast;

`Toast`

a
a [Toast](./kibana-plugin-public.toast.md)

2 changes: 1 addition & 1 deletion docs/development/core/public/kibana-plugin-public.toastsapi.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,5 +28,5 @@ export declare class ToastsApi implements IToasts
| [addSuccess(toastOrTitle)](./kibana-plugin-public.toastsapi.addsuccess.md) | | Adds a new toast pre-configured with the success color and check icon. |
| [addWarning(toastOrTitle)](./kibana-plugin-public.toastsapi.addwarning.md) | | Adds a new toast pre-configured with the warning color and help icon. |
| [get$()](./kibana-plugin-public.toastsapi.get_.md) | | Observable of the toast messages to show to the user. |
| [remove(toast)](./kibana-plugin-public.toastsapi.remove.md) | | Removes a toast from the current array of toasts if present. |
| [remove(toastOrId)](./kibana-plugin-public.toastsapi.remove.md) | | Removes a toast from the current array of toasts if present. |

4 changes: 2 additions & 2 deletions docs/development/core/public/kibana-plugin-public.toastsapi.remove.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,14 @@ Removes a toast from the current array of toasts if present.
<b>Signature:</b>

```typescript
remove(toast: Toast): void;
remove(toastOrId: Toast | string): void;
```

## Parameters

| Parameter | Type | Description |
| --- | --- | --- |
| toast | <code>Toast</code> | a returned by |
| toastOrId | <code>Toast &#124; string</code> | a [Toast](./kibana-plugin-public.toast.md) returned by [ToastsApi.add()](./kibana-plugin-public.toastsapi.add.md) or it's id |

<b>Returns:</b>

Expand Down
13 changes: 13 additions & 0 deletions docs/development/core/public/kibana-plugin-public.unmountcallback.md
View file
Open in desktop
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-public](./kibana-plugin-public.md) &gt; [UnmountCallback](./kibana-plugin-public.unmountcallback.md)

## UnmountCallback type

A function that will unmount the element previously mounted by the associated [MountPoint](./kibana-plugin-public.mountpoint.md)

<b>Signature:</b>

```typescript
export declare type UnmountCallback = () => void;
```
2 changes: 2 additions & 0 deletions src/core/public/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,8 @@ export {
ErrorToastOptions,
} from './notifications';

export { MountPoint, UnmountCallback } from './utils';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That seems to be the first time we have types in core that are used in different services (here both Notification and Overlay), so I did put them in utils. However not sure that's really the correct place. Maybe we want to create a core/public/common or core/public/shared ? WDYT ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is something we can expose from the Application Service, but still export them from the top-level (e.g. import { Mount } from 'src/core/public';. Right now the app service has interfaces for AppMountParameters and App which have similar properties:

export interface App extends AppBase {
  mount: (context: AppMountContext, params: AppMountParameters) => AppUnmount | Promise<AppUnmount>;
}

export interface AppMountParameters {
  element: HTMLElement;
}

We could have a generic Mount handler exported from there which can be inherited as well to solve these:

export type Unmount = () => void | Promise<() => void>;

export interface MountParameters {
  element: HTMLElement;
}

export type Mount<T extends MountParameters = MountParameters> = (params: T) => Unmount;

// These could now be used like:
title?: string | Mount;

// And I could use them in the Application Service as well:
export interface AppMountParameters extends MountParameters {
  // ...
}

export type AppMounter = Mount<AppMountParameters>;

Thoughts? cc: @joshdover

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to be sure I understand this correctly:

export interface App extends AppBase {
  mount: (context: AppMountContext, params: AppMountParameters) => AppUnmount | Promise<AppUnmount>;
}

would become

export interface AppMountParameters extends MountParameters {
  context: AppMountContext;
}

export type AppMounter = Mount<AppMountParameters>;

export interface App extends AppBase {
  mount: AppMounter;
}

Is that correct ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, as these type/interfaces are going to be imported from other core/public modules, is it the correct good practice to put them in core/public/application and import it from here in other modules (like core/public/notifications), or should we put it in a more 'shared' module ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm actually of the opinion we should hold off on doing this until we remove context in #49691. As it is right now, I don't think the examples above are actually quite compatible with the existing AppMount interface. No sense in breaking that interface twice, just to share types.

In terms of where to put the shared types now, I think a src/core/public/types file makes sense. We have a similar file on the server-side.

/**
* Core services exposed to the `Plugin` setup lifecycle
*
Expand Down
2 changes: 1 addition & 1 deletion src/core/public/notifications/toasts/__snapshots__/global_toast_list.test.tsx.snap
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions src/core/public/notifications/toasts/global_toast_list.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,9 @@ it('subscribes to toasts$ on mount and unsubscribes on unmount', () => {
it('passes latest value from toasts$ to <EuiGlobalToastList />', () => {
const el = shallow(
render({
toasts$: Rx.from([[], [1], [1, 2]]) as any,
toasts$: Rx.from([[], [{ id: 1 }], [{ id: 1 }, { id: 2 }]]) as any,
})
);

expect(el.find(EuiGlobalToastList).prop('toasts')).toEqual([1, 2]);
expect(el.find(EuiGlobalToastList).prop('toasts')).toEqual([{ id: 1 }, { id: 2 }]);
});
28 changes: 23 additions & 5 deletions src/core/public/notifications/toasts/global_toast_list.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,20 +17,38 @@
* under the License.
*/

import { EuiGlobalToastList, EuiGlobalToastListToast as Toast } from '@elastic/eui';

import { EuiGlobalToastList, EuiGlobalToastListToast as EuiToast } from '@elastic/eui';
import React from 'react';
import * as Rx from 'rxjs';
import { isString, omit } from 'lodash';
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with other commenters about removing reliance on isString.


import { MountWrapper } from '../../utils';
import { Toast } from './toasts_api';

interface Props {
toasts$: Rx.Observable<Toast[]>;
dismissToast: (t: Toast) => void;
dismissToast: (toastId: string) => void;
}

interface State {
toasts: Toast[];
}

const convertToEui = (toast: Toast): EuiToast => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be simplified by using spread, which should help eliminate the inline function and need for omit:

const convertToEui = (toast: Toast): EuiToast => ({
  ...toast,
  title: typeof toast.title === 'function' ? <MountWrapper mount={toast.title} /> : value,
  text: typeof toast.text === 'function' ? <MountWrapper mount={toast.text} /> : value,
});

Copy link
Contributor Author

@pgayvallet pgayvallet Oct 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

There is a subtle difference here: if title or text is not present (they are both optional), you will generate a mountWrapper mounting undefined instead or returning an object with no title or text property.

EDIT: err, you will generate an undefined title or text property instead of no property at all, which polute the object (and breaks snapshots). But I guess that's acceptable.

const wrap = (value: Toast['title'] | Toast['text']) =>
isString(value) || value === undefined ? value : <MountWrapper mount={value} />;
const euiToast: EuiToast = {
...omit(toast, ['title', 'text']),
};
if (toast.title !== undefined) {
euiToast.title = wrap(toast.title);
}
if (toast.text !== undefined) {
euiToast.text = wrap(toast.text);
}
return euiToast;
};

export class GlobalToastList extends React.Component<Props, State> {
public state: State = {
toasts: [],
Expand All @@ -54,8 +72,8 @@ export class GlobalToastList extends React.Component<Props, State> {
return (
<EuiGlobalToastList
data-test-subj="globalToastList"
toasts={this.state.toasts}
dismissToast={this.props.dismissToast}
toasts={this.state.toasts.map(convertToEui)}
dismissToast={toast => this.props.dismissToast(toast.id)}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we now got our own Toast type that's different from EUI, dismiss now also accepts the toast id to avoid falsy type conversion

Copy link
Contributor

Choose a reason for hiding this comment

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

Can shorten with a destructure:

dismissToast={({ id }) => this.props.dismissToast(id)}

/**
* This prop is overriden by the individual toasts that are added.
* Use `Infinity` here so that it's obvious a timeout hasn't been
Expand Down
10 changes: 8 additions & 2 deletions src/core/public/notifications/toasts/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,5 +18,11 @@
*/

export { ToastsService, ToastsSetup, ToastsStart } from './toasts_service';
export { ErrorToastOptions, ToastsApi, ToastInput, IToasts, ToastInputFields } from './toasts_api';
export { EuiGlobalToastListToast as Toast } from '@elastic/eui';
export {
ErrorToastOptions,
ToastsApi,
ToastInput,
IToasts,
ToastInputFields,
Toast,
} from './toasts_api';
4 changes: 2 additions & 2 deletions src/core/public/notifications/toasts/toasts_api.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ describe('#get$()', () => {
toasts.add('foo');
onToasts.mockClear();

toasts.remove({ id: 'bar' });
toasts.remove('bar');
expect(onToasts).not.toHaveBeenCalled();
});
});
Expand Down Expand Up @@ -136,7 +136,7 @@ describe('#remove()', () => {
it('ignores unknown toast', async () => {
const toasts = new ToastsApi(toastDeps());
toasts.add('Test');
toasts.remove({ id: 'foo' });
toasts.remove('foo');

const currentToasts = await getCurrentToasts(toasts);
expect(currentToasts).toHaveLength(1);
Expand Down
27 changes: 18 additions & 9 deletions src/core/public/notifications/toasts/toasts_api.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,13 @@
* under the License.
*/

import { EuiGlobalToastListToast as Toast } from '@elastic/eui';
import { EuiGlobalToastListToast as EuiToast } from '@elastic/eui';
import React from 'react';
import * as Rx from 'rxjs';
import { isString } from 'lodash';

import { ErrorToast } from './error_toast';
import { MountPoint, mountReact } from '../../utils';
import { UiSettingsClientContract } from '../../ui_settings';
import { OverlayStart } from '../../overlays';

Expand All @@ -33,13 +35,20 @@ import { OverlayStart } from '../../overlays';
*
* @public
*/
export type ToastInputFields = Pick<Toast, Exclude<keyof Toast, 'id'>>;
export type ToastInputFields = Pick<EuiToast, Exclude<keyof EuiToast, 'id' | 'text' | 'title'>> & {
title?: string | MountPoint;
text?: string | MountPoint;
};

export type Toast = ToastInputFields & {
id: string;
};

/**
* Inputs for {@link IToasts} APIs.
* @public
*/
export type ToastInput = string | ToastInputFields | Promise<ToastInputFields>;
export type ToastInput = string | ToastInputFields;
Comment on lines -42 to +51
Copy link
Contributor Author

@pgayvallet pgayvallet Oct 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The promise<> was unused, actually broken (somewhere in the code we object spread ToastInput assuming that's not a promise), and made some conversion quite hard, so I removed them. (and I don't think that really makes sense, in case of async toast, the waiting can be done in the caller code)


/**
* Options available for {@link IToasts} APIs.
Expand All @@ -59,13 +68,12 @@ export interface ErrorToastOptions {
toastMessage?: string;
}

const normalizeToast = (toastOrTitle: ToastInput) => {
const normalizeToast = (toastOrTitle: ToastInput): ToastInputFields => {
if (typeof toastOrTitle === 'string') {
return {
title: toastOrTitle,
};
}

return toastOrTitle;
};

Expand Down Expand Up @@ -123,11 +131,12 @@ export class ToastsApi implements IToasts {

/**
* Removes a toast from the current array of toasts if present.
* @param toast - a {@link Toast} returned by {@link ToastApi.add}
* @param toastOrId - a {@link Toast} returned by {@link ToastsApi.add} or it's id
Copy link
Contributor

Choose a reason for hiding this comment

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

s/it's/its/

*/
public remove(toast: Toast) {
public remove(toastOrId: Toast | string) {
const toRemove = isString(toastOrId) ? toastOrId : toastOrId.id;
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be inlined into the filter call since it is only used once:

const listWithoutToast = list.filter(t => t.id !== (typeof toastOrId === 'string' ? toastOrId : toastOrId.id));

const list = this.toasts$.getValue();
const listWithoutToast = list.filter(t => t !== toast);
const listWithoutToast = list.filter(t => t.id !== toRemove);
if (listWithoutToast.length !== list.length) {
this.toasts$.next(listWithoutToast);
}
Expand Down Expand Up @@ -191,7 +200,7 @@ export class ToastsApi implements IToasts {
iconType: 'alert',
title: options.title,
toastLifeTimeMs: this.uiSettings.get('notifications:lifetime:error'),
text: (
text: mountReact(
<ErrorToast
openModal={this.openModal.bind(this)}
error={error}
Expand Down
3 changes: 1 addition & 2 deletions src/core/public/notifications/toasts/toasts_service.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import React from 'react';
import { render, unmountComponentAtNode } from 'react-dom';

import { EuiGlobalToastListToast as Toast } from '@elastic/eui';
import { I18nStart } from '../../i18n';
import { UiSettingsClientContract } from '../../ui_settings';
import { GlobalToastList } from './global_toast_list';
Expand Down Expand Up @@ -65,7 +64,7 @@ export class ToastsService {
render(
<i18n.Context>
<GlobalToastList
dismissToast={(toast: Toast) => this.api!.remove(toast)}
dismissToast={(toastId: string) => this.api!.remove(toastId)}
toasts$={this.api!.get$()}
/>
</i18n.Context>,
Expand Down
Loading