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

Remove react references from core Notifications apis #49573

Merged
merged 25 commits into from
Nov 14, 2019

Conversation

pgayvallet
Copy link
Contributor

@pgayvallet pgayvallet commented Oct 28, 2019

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

For maintainers

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.

@pgayvallet 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
@elasticmachine
Copy link
Contributor

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

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

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@pgayvallet pgayvallet force-pushed the kbn-36425-agnostic-notifications branch from d9444ab to d5be874 Compare October 29, 2019 12:41
@pgayvallet pgayvallet force-pushed the kbn-36425-agnostic-notifications branch from d5be874 to 71f530b Compare October 29, 2019 12:41
@pgayvallet 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
Comment on lines 139 to 140
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.

Comment on lines 58 to 76
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)}

Comment on lines -42 to +51
export type ToastInput = string | ToastInputFields | Promise<ToastInputFields>;
export type ToastInput = string | ToastInputFields;
Copy link
Contributor Author

@pgayvallet pgayvallet Oct 29, 2019

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)

Comment on lines 25 to 26
export { CoreStart };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Comment on lines 37 to 40
title,
text: <>{body || null}</>,
title: isString(title) ? title : reactMount(title),
text: isString(body) ? body : reactMount(<>{body || null}</>),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will remove the useless check then.

Comment on lines 31 to 37
const mount = (element: HTMLElement) => {
ReactDOM.render(<I18nProvider>{component}</I18nProvider>, element);
return () => ReactDOM.unmountComponentAtNode(element);
};
// used for proper snapshot serialization
mount.__reactMount__ = component;
return mount;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Comment on lines -106 to +107
const reactComponent = (toastInput as any).text;
const wrapper = mountWithIntl(reactComponent);
const mountPoint = (toastInput as any).text;
const wrapper = mountWithIntl(mountPoint.__reactMount__);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Contributor

💚 Build Succeeded

@pgayvallet pgayvallet marked this pull request as ready for review October 29, 2019 13:56
@pgayvallet pgayvallet requested review from a team as code owners October 29, 2019 13:56
@pgayvallet
Copy link
Contributor Author

pgayvallet commented Nov 1, 2019

@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
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Changes for transform plugin LGTM


## MountPoint type

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

Choose a reason for hiding this comment

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

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.

@elasticmachine

This comment has been minimized.

@elasticmachine

This comment has been minimized.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

LGTM

@elasticmachine
Copy link
Contributor

💔 Build Failed

@pgayvallet

This comment has been minimized.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@pgayvallet
Copy link
Contributor Author

@eliperelman PTAL :)

Copy link
Contributor

@eliperelman eliperelman left a comment

Choose a reason for hiding this comment

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

Looks fantastic, great work!

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

Choose a reason for hiding this comment

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

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);

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@pgayvallet pgayvallet merged commit e04adbe into elastic:master Nov 14, 2019
pgayvallet added a commit to pgayvallet/kibana that referenced this pull request Nov 14, 2019
* 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
jloleysens added a commit to jloleysens/kibana that referenced this pull request Nov 14, 2019
* '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)
  ...
pgayvallet added a commit that referenced this pull request Nov 14, 2019
…#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
Copy link
Contributor

💚 Build Succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:New Platform release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor NotificationsService to not accept React components