Remove react references from core `Notifications` apis #49573

pgayvallet · 2019-10-28T22:38:07Z

Summary

Refactors core Notifications / Toasts apis to be framework agnostic.

Similar to #48431, but for the Notification core API (which only contains Toast ATM)

PR does the following:

Modify ToastsApi to no longer accept react components, and instead use the newly introduced MountPoint
Add the helper toMountPoint in kibana_react to convert from react node to MountPoint
Adapt the existing calls and unit tests
Add a custom snapshot serializer for toMountPoint
Updates generated doc

custom serializer for toMountPoint

Enzyme provides a proper snapshot serializer for react elements. As we switched from react elements to MountPoint, which are functions, that made the snapshot comparison kinda bogus, as every MountPoint serialize to [function ()]. So I added a custom serializer for react MountPoints, that render to something way more readable and diff-able.

for exemple when testing call snapshot to the Toast API, we had that kind of snapshots:

    "title": <FormattedMessage
      defaultMessage="Report may contain formulas {reportObjectType} '{reportObjectTitle}'"
      id="xpack.reporting.publicNotifier.csvContainsFormulas.formulaReportTitle"
      values={
        Object {
          "reportObjectTitle": "Yas",
          "reportObjectType": "yas",
        }
      }
    />,

Without the custom formatter, that converted to

    "title": [function ()],

With the formatter, that converts to

    "title": MountPoint {
      "reactNode": <FormattedMessage
        defaultMessage="Report may contain formulas {reportObjectType} '{reportObjectTitle}'"
        id="xpack.reporting.publicNotifier.csvContainsFormulas.formulaReportTitle"
        values={
          Object {
            "reportObjectTitle": "Yas",
            "reportObjectType": "yas",
          }
        }
      />,
    },

As react is the main framework used in kibana, most 'agnostic' calls are going to be done using the toMountPoint utility, so it really felt like this was very important for testability and readability.

Fixes #36425

Common bits with #48431

As both this and #48431 are not merged yet, the base code for MountPoint in core, reactMount in kibana-react and their utilities are present on both PR. I did not rebase on on the other as we don't know which one will be merged first.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~- [ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~

Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
~~- [ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

Dev Docs

The core NotificationService and ToastsApi methods are now framework agnostic and no longer accept react components as input. Please use kibana_react'stoMountPoint utility to convert a react node to a mountPoint.

elasticmachine · 2019-10-28T22:38:09Z

Pinging @elastic/kibana-platform (Team:Platform)

(kibana-react) properly export reactMount

(toast API) properly export new Toast type

createNotifications: do not wrap if text

fix xpack unit tests

pgayvallet · 2019-10-29T13:41:04Z

src/core/public/index.ts

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


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 ?

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

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 ?

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 ?

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.

pgayvallet · 2019-10-29T13:42:33Z

src/core/public/notifications/toasts/global_toast_list.tsx

-        dismissToast={this.props.dismissToast}
+        toasts={this.state.toasts.map(convertToEui)}
+        dismissToast={toast => this.props.dismissToast(toast.id)}


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

Can shorten with a destructure:

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

pgayvallet · 2019-10-29T13:44:57Z

src/core/public/notifications/toasts/toasts_api.tsx

-export type ToastInput = string | ToastInputFields | Promise<ToastInputFields>;
+export type ToastInput = string | ToastInputFields;


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)

pgayvallet · 2019-10-29T13:46:22Z

src/plugins/kibana_react/public/context/types.ts

-export { CoreStart };
-


Same issue as in #48431, please see here for reasons of this removal.

pgayvallet · 2019-10-29T13:47:31Z

src/plugins/kibana_react/public/notifications/create_notifications.tsx

-      title,
-      text: <>{body || null}</>,
+      title: isString(title) ? title : reactMount(title),
+      text: isString(body) ? body : reactMount(<>{body || null}</>),


Would work without this change, but this avoid using a MountWrapper when using actual string.

Maybe for simplicitly we can use reactMount for any case? If we don't, can we use standard type checking, like typeof title === 'string' instead importing something from lodash.

Agreed, the helper function should be able to handle this just fine.

Ok, will remove the useless check then.

pgayvallet · 2019-10-29T13:49:23Z

src/plugins/kibana_react/public/util/react_mount.tsx

+  const mount = (element: HTMLElement) => {
+    ReactDOM.render(<I18nProvider>{component}</I18nProvider>, element);
+    return () => ReactDOM.unmountComponentAtNode(element);
+  };
+  // used for proper snapshot serialization
+  mount.__reactMount__ = component;
+  return mount;


the __reactMount__ part is only using for tests and snapshots. We could remove it for production build, as it's private 'api'. Did not found any exemple of production code removal in kibana though, even if I know that's totally doable with webpack and env conditions. WDYT ?

I believe process.env.NODE_ENV === 'production' should work.

pgayvallet · 2019-10-29T13:51:15Z

x-pack/plugins/security/public/session/session_timeout.test.tsx

-    const reactComponent = (toastInput as any).text;
-    const wrapper = mountWithIntl(reactComponent);
+    const mountPoint = (toastInput as any).text;
+    const wrapper = mountWithIntl(mountPoint.__reactMount__);


the fact that we keep a reference to the react element in the mountpoint for serialization actually allows to do that. As the enzyme API is way better than manually mounting the mountpoint in tests (even if it does work), I think this trick is actually a good thing here.

elasticmachine · 2019-10-29T13:55:57Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 71f530b

pgayvallet · 2019-11-01T07:37:04Z

@eliperelman @joshdover I think all requested changes has been made. PTAL

Some points:

the core version of the adapter, mountReact is actually not only used in tests (we use it in ToastsApi#addError), so I did not move it to a test folder. Renamed it to mountReactNode however some something more explicit. It's not exported.
adapted the overlays/banner apis to use the new MountPoint types (was using their own duplicates, forget to refactor that)

EDIT: Also, as PR is effectively introducing breaking API changes in some of core API's, should I label this as breaking, or do we assume NP APIs are not yet officially used externally ?

elasticmachine · 2019-11-01T08:32:16Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 081f878

peteharverson

Changes for transform plugin LGTM

streamich · 2019-11-09T11:55:41Z

docs/development/core/public/kibana-plugin-public.mountpoint.md

+
+## MountPoint type
+
+A function that will should mount DOM content inside the provided container element and return a handler to unmount it.


Suggested change

A function that will should mount DOM content inside the provided container element and return a handler to unmount it.

A function that should mount DOM content inside the provided container element and return a handler to unmount it.

…c-notifications

elasticmachine · 2019-11-11T12:21:14Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 53d8da5

joshdover

LGTM

…c-notifications

elasticmachine · 2019-11-12T11:28:43Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: dad7ee6

…c-notifications

elasticmachine · 2019-11-12T22:35:24Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 5aede50

pgayvallet · 2019-11-13T07:47:51Z

@eliperelman PTAL :)

eliperelman

Looks fantastic, great work!

eliperelman · 2019-11-13T18:08:07Z

src/core/public/utils/mount.tsx

+export const mountReactNode = (component: React.ReactNode): MountPoint => (
+  element: HTMLElement
+) => {
+  ReactDOM.render(<I18nProvider>{component}</I18nProvider>, element);


nit: You can destructure these imports to make this a little more succinct.

import { render, unmountComponentAtNode } from 'react-dom'; // ... render(<I18nProvider>{component}</I18nProvider>, element); return () => unmountComponentAtNode(element);

…c-notifications

elasticmachine · 2019-11-14T07:39:45Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 912124c

* add reactMount util to kibana_react (kibana-react) properly export reactMount * add MountPoint types and utility * adapt toast API to no longer accept react elements (toast API) properly export new Toast type * adapt calls by using reactMount createNotifications: do not wrap if text * update generated doc * add custom snapshot serializer for reactMount * fix unit tests fix xpack unit tests * adapt non-ts calls * do not add __reactMount__ property in production * remove string check on createNotifications * fix typo and small fix using obj spread * improve react mount snapshot serializer * simplify convertToEui * rename reactMount to toMountPoint * adapt newly added calls * move mount types to proper file * use new Mount types for OverlayBanner apis * fixing typo * adapt new calls * use destructured imports

* 'master' of github.com:elastic/kibana: (27 commits) [Rollup] Fix for clone job workflow (elastic#50501) Empty message "No data available" for Labels and User metadata sections missing (elastic#49846) [APM] Duration by Country map doesn't take `transactionName` into account (elastic#50315) Remove react references from core `Notifications` apis (elastic#49573) Updated APM Indices endpoints to use the SavedObjectsClient from the legacy request context, and set the apm-indices schema object to be namspace-agnostic [Metrics UI] Calculate interval based on the dataset's period (elastic#50194) chore(NA): add new platform discovered plugins as entry points to check for dependencies on clean dll tasks (elastic#50610) [Telemetry] change of optin status telemetry (elastic#50158) [SIEM][Detection Engine] REST API Additions (elastic#50514) [DOCS] Removes dashboard-only mode doc (elastic#50441) [Filters] Fix operator overflowing out popover (elastic#50030) Change telemetry optIn to default to true (elastic#50490) [Maps] make grid rectangles the default symbolization for geo grid source (elastic#50169) Allow registered applications to hide Kibana chrome (elastic#49795) Upgrade EUI to v14.9.0 (elastic#49678) [Metrics UI] Convert layouts to use React components (elastic#49134) [Search service] Add support for ES request preference (elastic#49424) [Newsfeed/Lint] fix chained fn lint (elastic#50515) [Monitoring] Fix logstash pipelines page in multi-cluster environment (elastic#50166) [SIEM] Events viewer fixes (elastic#50175) ...

…#50616) * Remove react references from core `Notifications` apis (#49573) * add reactMount util to kibana_react (kibana-react) properly export reactMount * add MountPoint types and utility * adapt toast API to no longer accept react elements (toast API) properly export new Toast type * adapt calls by using reactMount createNotifications: do not wrap if text * update generated doc * add custom snapshot serializer for reactMount * fix unit tests fix xpack unit tests * adapt non-ts calls * do not add __reactMount__ property in production * remove string check on createNotifications * fix typo and small fix using obj spread * improve react mount snapshot serializer * simplify convertToEui * rename reactMount to toMountPoint * adapt newly added calls * move mount types to proper file * use new Mount types for OverlayBanner apis * fixing typo * adapt new calls * use destructured imports * adapt call in 7.x

elasticmachine · 2019-11-14T21:15:41Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 912124c

pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform v8.0.0 v7.6.0 labels Oct 28, 2019

pgayvallet force-pushed the kbn-36425-agnostic-notifications branch from 6840ed2 to 13399f4 Compare October 28, 2019 23:15

This comment has been minimized.

Sign in to view

pgayvallet force-pushed the kbn-36425-agnostic-notifications branch from d9444ab to d5be874 Compare October 29, 2019 12:41

pgayvallet added 8 commits October 29, 2019 13:41

add reactMount util to kibana_react

688c5a2

(kibana-react) properly export reactMount

add MountPoint types and utility

678eb13

adapt toast API to no longer accept react elements

6b986ee

(toast API) properly export new Toast type

adapt calls by using reactMount

5464cb2

createNotifications: do not wrap if text

update generated doc

ee61086

add custom snapshot serializer for reactMount

bd036bc

fix unit tests

a49de40

fix xpack unit tests

adapt non-ts calls

71f530b

pgayvallet force-pushed the kbn-36425-agnostic-notifications branch from d5be874 to 71f530b Compare October 29, 2019 12:41

pgayvallet added the release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. label Oct 29, 2019

pgayvallet commented Oct 29, 2019

View reviewed changes

pgayvallet marked this pull request as ready for review October 29, 2019 13:56

pgayvallet requested review from a team as code owners October 29, 2019 13:56

peteharverson approved these changes Nov 1, 2019

View reviewed changes

streamich reviewed Nov 9, 2019

View reviewed changes

fixing typo

7fa9e5d

pgayvallet mentioned this pull request Nov 11, 2019

Remove react references from core OverlayService apis #48431

Merged

3 tasks

Merge remote-tracking branch 'upstream/master' into kbn-36425-agnosti…

53d8da5

…c-notifications

This comment has been minimized.

Sign in to view

joshdover approved these changes Nov 11, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into kbn-36425-agnosti…

dad7ee6

…c-notifications

This comment has been minimized.

Sign in to view

pgayvallet added 2 commits November 12, 2019 22:12

Merge remote-tracking branch 'upstream/master' into kbn-36425-agnosti…

b944611

…c-notifications

adapt new calls

5aede50

eliperelman approved these changes Nov 13, 2019

View reviewed changes

pgayvallet added 2 commits November 14, 2019 07:23

Merge remote-tracking branch 'upstream/master' into kbn-36425-agnosti…

c869fea

…c-notifications

use destructured imports

912124c

pgayvallet merged commit e04adbe into elastic:master Nov 14, 2019

pgayvallet mentioned this pull request Nov 14, 2019

[7.x] Remove react references from core Notifications apis (#49573) #50616

Merged

		export type ToastInput = string \| ToastInputFields \| Promise<ToastInputFields>;
		export type ToastInput = string \| ToastInputFields;


		## MountPoint type

		A function that will should mount DOM content inside the provided container element and return a handler to unmount it.

	A function that will should mount DOM content inside the provided container element and return a handler to unmount it.
	A function that should mount DOM content inside the provided container element and return a handler to unmount it.

Remove react references from core Notifications apis #49573

Remove react references from core Notifications apis #49573

Conversation

pgayvallet commented Oct 28, 2019 • edited by rayafratkina Loading

Summary

custom serializer for toMountPoint

Common bits with #48431

Checklist

For maintainers

Dev Docs

elasticmachine commented Oct 28, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet Oct 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Oct 29, 2019

💚 Build Succeeded

pgayvallet commented Nov 1, 2019 • edited Loading

elasticmachine commented Nov 1, 2019

💚 Build Succeeded

peteharverson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

elasticmachine commented Nov 11, 2019

💚 Build Succeeded

joshdover left a comment

Choose a reason for hiding this comment

elasticmachine commented Nov 12, 2019

💔 Build Failed

This comment has been minimized.

elasticmachine commented Nov 12, 2019

💚 Build Succeeded

pgayvallet commented Nov 13, 2019

eliperelman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Nov 14, 2019

💚 Build Succeeded

elasticmachine commented Nov 14, 2019

💚 Build Succeeded

Remove react references from core `Notifications` apis #49573

Remove react references from core `Notifications` apis #49573

pgayvallet commented Oct 28, 2019 •

edited by rayafratkina

Loading

pgayvallet Oct 29, 2019 •

edited

Loading

pgayvallet commented Nov 1, 2019 •

edited

Loading