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

Adds toast ID to toast api #3752

Merged
merged 6 commits into from
Apr 17, 2023
Merged
Changes from 4 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -71,6 +71,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- Use mirrors to download Node.js binaries to escape sporadic 404 errors ([#3619](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3619))
- [Multiple DataSource] Refactor dev tool console to use opensearch-js client to send requests ([#3544](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3544))
- [Data] Add geo shape filter field ([#3605](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3605))
- [Notifications] Adds id to toast api for deduplication ([#3752](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3752))
- [Dashboard] Indicate that IE is no longer supported ([#3641](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3641))
- [UI] Add support for comma delimiters in the global filter bar ([#3686](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3686))
- [Multiple DataSource] Allow create and distinguish index pattern with same name but from different datasources ([#3571](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3571))
23 changes: 23 additions & 0 deletions src/core/public/notifications/toasts/toasts_api.test.ts
Original file line number Diff line number Diff line change
@@ -105,6 +105,24 @@ describe('#get$()', () => {
toasts.remove('bar');
expect(onToasts).not.toHaveBeenCalled();
});

it('does not emit a new toast list when a toast with the same id is passed to add()', () => {
const toasts = new ToastsApi(toastDeps());
const onToasts = jest.fn();

toasts.get$().subscribe(onToasts);
toasts.add({
id: 'foo',
title: 'foo',
});
onToasts.mockClear();

toasts.add({
id: 'foo',
title: 'bar',
});
expect(onToasts).not.toHaveBeenCalled();
});
});

describe('#add()', () => {
@@ -135,6 +153,11 @@ describe('#add()', () => {
const toasts = new ToastsApi(toastDeps());
expect(toasts.add('foo')).toHaveProperty('title', 'foo');
});

it('accepts an id and does not auto increment', async () => {
const toasts = new ToastsApi(toastDeps());
expect(toasts.add({ id: 'foo', title: 'not foo' })).toHaveProperty('id', 'foo');
});
});

describe('#remove()', () => {
13 changes: 10 additions & 3 deletions src/core/public/notifications/toasts/toasts_api.tsx
Original file line number Diff line number Diff line change
@@ -42,12 +42,10 @@ import { I18nStart } from '../../i18n';
/**
* Allowed fields for {@link ToastInput}.
*
* @remarks
* `id` cannot be specified.
Copy link
Member

Choose a reason for hiding this comment

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

do we know the historical reason why?

Copy link
Member Author

Choose a reason for hiding this comment

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

I looked at the git history but as far as I can tell, there isn't a good reason. That's why I kept the default behavior as is. Only if you explicitly pass an id does the behavior change

Copy link
Member

Choose a reason for hiding this comment

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

After reviewing, my understanding is that it was just intended to be an internal property, that simply auto-increments with each new toast. Ashwin's change here changes the purpose and usage of the property a bit (for example, passing string ids instead of a number), but not in a way that affects the historical usage. So I don't think it was intended as "specifying id is prohibited" and instead meant "specifying id is not necessary".

*
* @public
*/
export type ToastInputFields = Pick<EuiToast, Exclude<keyof EuiToast, 'id' | 'text' | 'title'>> & {
id?: string;
title?: string | MountPoint;
text?: string | MountPoint;
};
@@ -143,6 +141,15 @@ export class ToastsApi implements IToasts {
* @returns a {@link Toast}
*/
public add(toastOrTitle: ToastInput) {
if (typeof toastOrTitle !== 'string') {
const list = this.toasts$.getValue();
const existingToast = list.find((toast) => toast.id === toastOrTitle.id);
ashwin-pc marked this conversation as resolved.
Show resolved Hide resolved

if (existingToast) {
return existingToast;
}
}

const toast: Toast = {
id: String(this.idCounter++),
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it's somewhat surprising that we still increment the idCounter, but throw away the value. On the one hand, it means that the counter really does match the total number, but on the other, there are now some idCounter values that don't exist in the map of toast IDs.

Copy link
Member Author

Choose a reason for hiding this comment

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

We dont still increment, if an existing toast is found, we exit this function when we return the existing toast.

Copy link
Member

Choose a reason for hiding this comment

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

Right, I'm talking about the case where an id is provided, but there's no existing matching toast. In that case we increment, but don't use the incremented value.

toastLifeTimeMs: this.uiSettings.get('notifications:lifetime:info'),
Original file line number Diff line number Diff line change
@@ -51,7 +51,12 @@ export const Workspace: FC = ({ children }) => {
if (noAggs || !aggValidation.valid || !schemaValidation.valid) {
const err = schemaValidation.errorMsg || aggValidation.errorMsg;

if (err) toasts.addWarning(err);
if (err)
toasts.addWarning({
id: 'vb_expression_validation',
title: err,
});

setExpression(undefined);

return;
Original file line number Diff line number Diff line change
@@ -47,9 +47,9 @@ export const getPreloadedStore = async (services: VisBuilderServices) => {
};

// the store subscriber will automatically detect changes and call handleChange function
store.subscribe(handleChange);
const unsubscribe = store.subscribe(handleChange);
joshuarrrr marked this conversation as resolved.
Show resolved Hide resolved

return store;
return { store, unsubscribe };
};

// Infer the `RootState` and `AppDispatch` types from the store itself
3 changes: 2 additions & 1 deletion src/plugins/vis_builder/public/plugin.ts
Original file line number Diff line number Diff line change
@@ -161,14 +161,15 @@ export class VisBuilderPlugin
};

// Instantiate the store
const store = await getPreloadedStore(services);
const { store, unsubscribe: unsubscribeStore } = await getPreloadedStore(services);
const unmount = renderApp(params, services, store);

// Render the application
return () => {
unlistenParentHistory();
unmount();
appUnMounted();
unsubscribeStore();
};
},
});