[new-platform] Refactor OverlayService to not require React to use

[joshdover]

Currently, the OverlayService exposes a React interface from Core. All APIs in Core should be Vanilla JavaScript and not dependent on any specific rendering framework, even if they use one under the hood.

We need to fix this inconsistency, here's my proposed interface.

// Before
export interface OverlayStart {
  openFlyout: (
    flyoutChildren: React.ReactNode,
    flyoutProps?: {
      closeButtonAriaLabel?: string;
      'data-test-subj'?: string;
    }
  ) => OverlayRef;
  openModal: (
    modalChildren: React.ReactNode,
    modalProps?: {
      closeButtonAriaLabel?: string;
      'data-test-subj'?: string;
    }
  ) => OverlayRef;
}

// After
export interface OverlayStart {
  openFlyout: (
    // `render` returns a function used to unmount whatever is rendered at the DOM node.
    render: (targetDomNode: HTMLElement) => () => void,
    options?: {
      closeButtonAriaLabel?: string;
      'data-test-subj'?: string;
    }
  ) => OverlayRef;
  openModal: (
    render: (targetDomNode: HTMLElement) => () => void,
    options?: {
      closeButtonAriaLabel?: string;
      'data-test-subj'?: string;
    }
  ) => OverlayRef;
}

[elasticmachine]

Pinging @elastic/kibana-platform

[streamich]

OverlayRef sounds like something that could still be React-specific.

I was suggesting here this interface could be standartized, for example, called MountPoint.

export interface OverlayStart {
  openFlyout: MountPoint;
  openModal: MountPoint;
}

and for the Application service, too

export interface ApplicationSetup {
  renderIcon: MountPoint;
  renderApp: MountPoint;
}

[pgayvallet]

We want something framework agnostic, but React is render-target agnostic.
I'm not sure it's possible to get an actual HTMLElement from an unmounted react component ?

findDOMNode only works on mounted components (that is, components that have been placed in the DOM).

If we take an example (src/legacy/ui/public/courier/fetch/components/shard_failure_open_modal_button.tsx):

const modal = npStart.core.overlays.openModal(
      <ShardFailureModal
        request={request}
        response={response}
        title={title}
        onClose={() => modal.close()}
      />,
      {
        className: 'shardFailureModal',
      }
    );

What would be the conversion, does something like works ?

let modal = null;
const modalContent = (<ShardFailureModal
        request={request}
        response={response}
        title={title}
        onClose={() => modal.close()}
      />)

const wrapper = document.createElement('div');
ReactDOM.render(modalContent, wrapper);
modal = npStart.core.overlays.openModal(wrapper, {});

Maybe the responsibilities should be reversed? I'm thinking of something like this:

modal = npStart.core.overlays.openModal(
    (modalRoot: HTMLElement) => ReactDOM.render(modalContent, modalRoot), {}
);

Or am I missing an easier way to actually get a ref from an unmounted react element ?

[pgayvallet]

As @cjcenizal explains here: #36425 (comment), the underlying EUI components we are using are React components. That's why currently, using this framework is the easiest way to interact with modals, flyover and toasts.

We can probably (and need to) add an abstraction level, but even in that case, I think keeping the possibility to actually use 'native' React element will be the best way to not impact the developer experience (asking to migrate to React while only exposing agnostic UI APIs seems questionnable to me).

If we see my previous exemple, there is a difference between

const modal = npStart.core.overlays.openModal(<MyComponent/>);

and

const modalContent = <MyComponent/>;
npStart.core.overlays.openModal( 
   (modalRoot: HTMLElement) => ReactDOM.render(modalContent, modalRoot), {})
)

More importantly, as long as we keep using EUI components as our implementation, any non-react content passed to any modal, toast or flyover will need to be converted back into react world, and the only option for this is to use dangerouslySetInnerHTML to use a 'bridge' component which inject back the vanilla JS using an inner ref, which should be avoided when possible. That's why I don't think that forcing the react -> vanilla html -> react is really something we want to do if we are not forced to.

However, I agree that we still needs to allow developers to use any other framework that React, and we need to extend the current apis.

That's why I'd like to propose something slightly different than your interface @joshdover:

// naming open to proposal, not really fond of this
type ManualRender = (parent: HTMLElement) => void;
type DomContent = React.ReactNode | HTMLElement | ManualRender | string; // do we want to allow stringified html ?

export interface OverlayStart {
  /** {@link OverlayBannersStart} */
  banners: OverlayBannersStart;
  openFlyout: (
    flyoutChildren: DomContent,
    flyoutProps?: {
      closeButtonAriaLabel?: string;
      'data-test-subj'?: string;
    }
  ) => OverlayRef;
  openModal: (
    modalChildren: DomContent,
    modalProps?: {
      className?: string;
      closeButtonAriaLabel?: string;
      'data-test-subj'?: string;
    }
  ) => OverlayRef;
}

That way we keep React as a first-class citizen of the NP, while staying agnostic by allowing other way to use these services without react.

Regarding the name overlayRef, I don't think the term reference is exclusive to React world (and in this specific case, it's not, the ref we return is a vanilla object), so I'm not sure to see any reason to rename it.

[joshdover]

We talked offline, but for now, we're going to stick with the raw DOM element approach to fulfill these 3 goals:

• Framework-agnostic Core
• As few APIs as possible to reduce maintenance burden
• Consistency with other Core APIs

How we improve the ergonomics here is something we should discuss, but whatever we adopt should be something we support on all Core APIs.

[pgayvallet]

To keep the framework agnostic approach, we will then re-use the types already defined in the banner service (with proper renaming to something more generic. @streamich suggestion of MountPoint seems great.

type Unmount = () => void;
type MountPoint = (element: HTMLElement) => Unmount;

not sure how to properly name the Unmount type though.

[cjcenizal]

not sure how to properly name the Unmount type though.

UnmountCallback?