[discuss] Direct dependency on NP code in low-level components...?

[clintandrewhall]

As more and more code is migrated to the New Platform, I'm seeing a trend in low level, oftentimes shared components that is causing complications. To be fair, this is a problem as old as Flux/Redux/Relay/etc... but we should discuss our best practices, mitigations... indeed, even if we feel this is a problem.

Problem Statement

New Platform utilities provide single-import access to contextual data throughout Kibana. When low-level, potentially shared components include code from the New Platform directly, it introduces a contextual dependency that may or may not be immediately clear. Without a strong mock or default, dependent code could find itself managing contextual values of which it has no interest or control.

While at the moment these issues only appear when trying to use components outside of Kibana, I'm afraid if we don't make these contracts and dependencies clear we'll introduce a great deal of instability across Kibana plugins.

Proposals

We could do nothing at all. We can assume that all code within Kibana runs within Kibana and is provided the context it expects. But using testing tools beyond Jest, externalizing components or creating products outside of Kibana would certainly break.

Case in point: the bespoke Canvas Shareable Runtime allows Canvas Workpads to be embedded in external websites. At the moment, it's working fine, and has high unit test coverage. But if a single component dependency starts to use the NP, it will certainly break, and it won't be obvious why.

One option would be to abstract the NP context defaults outside of Jest and allow a mechanism to provide that default easily. Another would be to have Kibana plugins provide a configuration or manifest of dependent contextual values.

I don't have a lot of experience with the NP, so I thought I'd file this for discussion.

Examples

There are several examples in code today, some benign, others less so.

Code Editor

@poffdeluxe recently moved the Monaco-based code editor to kibana_react. Mitigating the deep dependency on NP required knowing and building the relevant entries.

Issue

1. The base component, code_editor.tsx, is wrapped in a simple container which retreives and provides the theme value from the NP:

import { useUiSetting } from '../ui_settings';
...
export const CodeEditor: React.FunctionComponent<Props> = props => {
  const darkMode = useUiSetting<boolean>('theme:darkMode');
  return <BaseEditor {...props} useDarkTheme={darkMode} />;
};

2. This container is consumed by Canvas in the expression_input component.

3. I then used expression_input in a POC outside of Kibana, in a Storybook instance. It promptly blew up, and it wasn't clear at all why:

Mitigation: #52209

First, we tried to use the coreMock provided by the NP:

// Add New Platform Context for any stories that need it    
addDecorator(fn => (
  <KibanaContextProvider services={coreMock.createStart()}>
    {fn()}
  </KibanaContextProvider>
));

This failed, as Storybook does not use Jest. In the end we mocked the UI Setting manually:

const services = new Map([['darkMode', true]]);
addDecorator(fn => (
  <KibanaContextProvider services={services}>
    {fn()}
  </KibanaContextProvider>
));

Canvas Embeddables

TL;DR A render expression function for Embeddables uses NP for a number of props. This function had to be blacklisted to continue development, as the dependency values required deep understanding.

Issue

1. The embeddable renderer function in Canvas uses a number of NP-ready components from Kibana.

2. embeddable is included in the collection of renderer functions.

3. Importing the renderer collection caused a number of obtuse errors in the Storybook POC, starting with a dependency on i18n:

Mitigation

As you can imagine, simply overcoming the lack of i18n wasn't enough: lots of NP values were missing. To resolve, I added null-loader entry for embeddable.tsx for Storybook:

config.module.rules.push({
  test: path.resolve(__dirname, '../canvas_plugin_src/renderers/embeddable.tsx'),
  use: 'null-loader',
});

Further Considerations

I'm using the Interpreter in my POC. If I load my components within a Kibana plugin, it works as expected. To iterate and test within Storybook, or to deploy my POC as a standalone solution, I have to instantiate the Expressions plugin manually. It would be great to have an alternative:

let interpretAstFn: ExpressionInterpretWithHandlers | null = null;

const getInterpretAstFn = (functionDefinitions: CanvasFunction[]) => {
  if (!interpretAstFn) {
    const { expressions } = npSetup.plugins;

    if (expressions) {
      interpretAstFn = expressions.__LEGACY.getExecutor().interpreter.interpretAst;
    } else {
      const placeholder = {} as any;
      const expressionsPlugin = plugin(placeholder);
      const setup = expressionsPlugin.setup(placeholder, {
        inspector: {},
      } as any);
      const { getExecutor, functions, renderers } = setup.__LEGACY;
      functionDefinitions.forEach(fn => functions.register(fn));
      renderFunctions.forEach((fn: ExpressionRenderDefinition) => {
        if (fn) {
          renderers.register(fn);
        }
      });

      interpretAstFn = getExecutor().interpreter.interpretAst;
    }
  }
...
...

cc: @poffdeluxe @streamich @lukeelmers @stacey-gammon

[elasticmachine]

Pinging @elastic/kibana-app-arch (Team:AppArch)

[elasticmachine]

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

[clintandrewhall]

It was suggested app and app-arch might be interested in weighing in... added tags for visibility. Remove if not applicable, thanks!

[streamich]

We could implement the core system in-memory

• fatalErrors — could do nothing.
• injectedMetadata — would be a mock that holds most recent injected metadata.
• legacy — would not exist.
• notifications — existing service can be used.
• http — would do nothing or could still execute some HTTP requests.
• savedObjects — would store saved objects in memory, and fetch them from memory.
• uiSettings — would store settings in memory, and fetch from memory.
• chrome — most of the current implementation could be probably re-used.
• i18n — can be mocked.
• overlay — can be used as is.
• plugins — would not exist. This service is used to load plugins; in-memory plugins could be loaded manually, without auto-discovery.
• application — existing service can probably be used.
• docLinks — existing service can be used.
• rendering — not needed.
• context — will not exist in the future, but if it did, existing implementation could be used.
• integratons — not relevant.

The biggest ones to implement in memory would be savedObjects and uiSettings that would store data in-memory, only for duration while those services exist. And we would need to figure some sensible mock for http service. Other services seem to be straight-forward, or even existing implementations could be used.

Once we have core implemented in-memory, we could already bootstrap most of the plugins without any modifications needed. (For example, plugins mentioned by OP, embeddable and expressions would probably already boot without any changes). Probably, most of the rest of the plugins we could still boot with some tweaks.

That would give us an ability to create in-memory contracts at will:

const { core, plugins } = createInMemoryStartContracts();

Use the services mentioned by OP:

core.uiSettings.get(...)
plugins.embeddable
plugins.expressions.__LEGACY.getExecutor().interpreter.interpretAst

We could create as many instances of those contracts as needed in microseconds:

createInMemoryStartContracts();
createInMemoryStartContracts();
createInMemoryStartContracts();

How is that useful?

• In-memory contracts can be used in tests, to run realistic scenarios. Each unit test could have its own set of in-memory contracts.
• They would allow to render in Storybook virtually any part of Kibana, possibly even whole apps or even the whole Kibana.
• They would allow to create "shareable runtime", like the one Canvas created, that can render any piece of Kibana on external website.
• Same tests that are run against the real core services could be run agains the in-memory implementations for extra stability.

This could be a nice Space Time project.

[joshdover]

We could implement the core system in-memory

While I think there could be some value with this option, I think the need for this actually exposes a larger architectural problem that arises in UI plugins as they grow: very tight coupling to Core APIs. In addition, I think the maintenance burden of an in-memory implementation of Core is quite high and very prone to bugs or inconsistencies with the real implementation.

A large UI tree that is tightly coupled to Kibana's Core API is going to have other problems not mentioned in the OP:

• Changing Core APIs will break the UI very frequently
• Testing the UI is much more complicated and requires more sophisticated mocks
• Moving or sharing parts of the UI tree in different places (incl. Storybook) will continue to present problems with dependencies, quite often requiring refactoring.

These are large problems with high maintenance costs that will slow down not only Canvas (and other apps) but the entire Kibana Platform from improving. It seems to me the best solution is to decouple the UI from Core APIs as much as possible. How could we do this?

My gut reaction is to abstract away data-access and integration points with Core from the UI itself. There are many options to do this, but I believe a state management framework like Redux, MobX, or similar is the best option. By moving all of your integrations with Core into plain JavaScript, you can simplify your UI's dependencies significantly. This allow for increased portability of the UI code & easier adoption of API changes (because there is only a single place where you directly interface with Core).

Example

Using the above example about a UI component's dependency on UiSettings:

import { useUiSetting } from '../ui_settings';
...
export const CodeEditor: React.FunctionComponent<Props> = props => {
  const darkMode = useUiSetting<boolean>('theme:darkMode');
  return <BaseEditor {...props} useDarkTheme={darkMode} />;
};

This could be refactored to have a very simple dependency on a data structure that is sourced from a Redux store:

import { connect } from 'react-redux'

export const CodeEditorUi: React.FunctionComponent<Props> = props => {
  return <BaseEditor {...props} useDarkTheme={props.darkMode} />;
};

export const CodeEditor = connect(
  store => ({ darkMode: store.uiSettings.darkMode })
)(CodeEditorUi);

Benefits:

• The UI component can easily be reused in Storybook
• The integration logic with Core can be moved to a Redux action creator that is easily tested
• Changing where the darkMode value is sourced from is trivial, including adapting to breaking changes in the Core API
• No risk of using mocks that have inconsistent behavior with Core APIs

Downsides:

• More work on each plugin to pull apart Core API usages from UI components

[clintandrewhall]

This is a great idea, but I'd propose we use React useContext instead. It's already present in all plugins, and prevents us from locking into a dependency.

Still... this leaves the underlying problem, namely that components that use CodeEditor that are consumed by other components are not aware that a context is being used that far down the tree. Mocking, or having a manifest of all contextual values, is still going to need to happen:

/kibanaReact: const CodeEditorUI = <BaseEditor />;
/kibanaReact: const CodeEditor = connect(<CodeEditorUi />);
/kibanaReact: const Panel = () => <CodeEditor />;
/canvas: const CanvasWorkpadUI = () => <Panel />;
/canvas: const CanvasWorkpad = () => <CanvasWorkpadUI />;
/someNewThing: const SomeNewCanvasProductUI = () => <CanvasWorkpad />;
/test: const StorybookStoryOfNewProduct = () => <SomeNewCanvasProduct />;

ERROR: uiSettings/darkMode/someOtherThing is not defined.

[clintandrewhall]

Something else that's good to know about useContext: it accepts a default when it's created:

https://reactjs.org/docs/hooks-reference.html#usecontext

[clintandrewhall]

Ok, I've been thinking about this some more, and perhaps we should consider custom hooks for each property, rather a larger getter that retrieves a text-based property. It will make grepping use easier, and components themselves can define their defaults.

import { useDarkMode } from '../ui_settings/theme';
...
export const CodeEditor: React.FunctionComponent<Props> = props => {
  const darkMode = useDarkMode('false');
  return <BaseEditor {...props} useDarkTheme={darkMode} />;
};

I'd also propose we dump containers, as well... see Abramov's comment on this post.

[joshdover]

This is a great idea, but I'd propose we use React useContext instead. It's already present in all plugins, and prevents us from locking into a dependency.

I'm not sure I follow. I think React.Context does exactly that and lock you into a dependency.

Still... this leaves the underlying problem, namely that components that use CodeEditor that are consumed by other components are not aware that a context is being used that far down the tree. Mocking, or having a manifest of all contextual values, is still going to need to happen:

I think what you're battling here is one of the flaws with React.Context. Though you could de-couple the UI component from the connected component that depends on the context, very similar to my Redux example.

import { connect } from 'react-redux'

/** Pure UI component that has no Platform dependencies or Context dependency */
export const CodeEditorUi: React.FunctionComponent<Props> = props => {
  return <BaseEditor {...props} useDarkTheme={props.darkMode} />;
};

/** Connected component which wires the UI flavor to a specific context dependency */
export const CodeEditor: React.FunctionComponent<Props> = props => {
  const { uiSettings } = useContext();
  return <CodeEditorUi darkMode={uiSettings.darkMode} {...props} />;
};

[joshdover]

Regarding the post you linked to:

But I’ve seen it enforced without any necessity and with almost dogmatic fervor far too many times. The main reason I found it useful was because it let me separate complex stateful logic from other aspects of the component. Hooks let me do the same thing without an arbitrary division.

I think the distinction between his statement and our situation is that we do have a necessity: we want to use the same presentational components in places where the state we rely on is not available (eg. Storybook). In addition, we want to be able to change how we get that state very easily, without having to update every UI component in Kibana.

[joshdover]

This is a great idea, but I'd propose we use React useContext instead. It's already present in all plugins, and prevents us from locking into a dependency.

Thinking about this more, I think that the problem here isn't the Context pattern itself, it's what is in your context that is important.

If you're exposing any complex, raw Core APIs (eg. SavedObjects, UiSettings, etc.) to low-level UI components, you're going to be coupling your presentational code very tightly to these APIs. This is what causes the problems we've discussed above.

What would be best is if the context your UI components depended on, only contained the values they needed to render + interact with state in Core. For example:

const CanvasContext = React.createContext();

// Instead of exposing all of CoreStart to your UI, just expose the parts you need
const CanvasContextProvider = (props: { core: CoreStart }) => {
  const darkMode = useObservable(core.uiSettings.client.get$('theme:darkMode'));
  const saveWorkpad = workpad => core.savedObjects.client.save(/** args */);
  return <CanvasContext.Provider value={{ darkMode, saveWorkpad }} />;
};

// For Storybook, create a really dumb provider that does very little
const MockCanvasContextProvider = () => {
  const saveWorkpad = workpad => Promise.resolve(workpad);
  return <CanvasContext.Provider value={{ darkMode: false, saveWorkpad }} />;
};

[joshdover]

Talked to @clintandrewhall today.

Both agreed that wrapping Core APIs with purpose-built hooks or a purpose-built context provider both get us to a state where we're decoupled from Core APIs. ✅

There are some (mostly minor) tradeoffs between the two approaches:

• Hooks feel a bit more idiomatic React than context + useContext hook
• Mocking hooks may be more complex than providing a mock context provider
• If a context provider gets very large, it may be harder to change and test. Hooks provide a bit more isolation here. Maybe this could be solved with multiple context providers?

Next steps:

• It would be great if we could converge on some best practices and blessed conventions here for the entire codebase.
• We should consider deprecating and removing the KibanaContextProvider in kibana-react. It encourages the tight coupling issues outlined in this issue. Tight coupling of deeply-nested UI components is going to make refactoring and adapting to Core API changes incredibly difficult.
• Once we converge on an approach, it may be beneficial to provide a common toolbelt to make this pattern easy. For example, shared library of hooks, or a shared library of redux action creators + reducers.

\cc @elastic/kibana-platform @elastic/kibana-app-arch

[pgayvallet]

We should consider deprecating and removing the KibanaContextProvider in kibana-react. It encourages the tight coupling issues outlined in this issue. Tight coupling of deeply-nested UI components is going to make refactoring and adapting to Core API changes incredibly difficult.

Following my slack discussion with josh, I can't stress enough how I think this is crucial for the sanity of the codebase.

[lukeelmers]

We discussed this topic as a team yesterday, and the general conclusion was:

• We see the ways that KibanaContextProvider can cause tight coupling which probably isn't the best long-term solution for maintaining UIs
• We agree that either purpose-built hooks or purpose-built context providers would address this issue
• We also don't want to remove KibanaContextProvider until we have a proven alternative that have worked for teams

So the next steps are basically what Josh outlines above:

• Align on best practices for this situation
• Document those and label KibanaContextProvider as deprecated
• Later revisit what additional tooling would be useful

I'll create issues to track these tasks in the next few days pending any further input from folks on this thread. (cc @streamich in particular would value any additional comments you have here since you have been closest to this)

Personally I think I prefer the purpose-built context provider option, for the ease of testing and for the practical reason that it doesn't necessitate maintaining a library of hooks that match all of the core services (this is something I was keen to avoid when we initially rolled out kibana_react, as it is officially owned by app arch). I like the idea of keeping core "closer" to the apps for now rather than abstracting too much, especially since the new platform is still, well... new. There is always room to abstract more later.

[lukeelmers]

App Arch took another look at this, and the current KibanaContextProvider should let you pass anything you want into the provider itself, e.g.:

<KibanaContextProvider services={{ notifications, overlays, embeddable }}>
  <KibanaApplication />
</KibanaContextProvider>

As a result, the team felt that as a next step, rather than completely deprecating the provider, we should instead update our documentation to remove examples where all of core is being passed in. Instead, we'll only show examples where you select the pieces of core that you depend on.

I will close this issue for now since we are going to track follow-up steps in https://github.com/elastic/kibana/issues/52494, but feel free to re-open if there are still unanswered questions.