Lens core restructuring

[chrisdavies]

I've done a sort of high-level code review of Lens, and think we'd benefit from some core changes. This list captures the things that most stood out to me.

Outline

• Use a single, root state
• Move plugin registration to the root
• Avoid useEffect
• Use rxjs observables in a data module (or maybe a data module per plugin as need be) to replace all occurrences of useEffect
  • lens$: an observable of standard Lens state
  • state$: an observable of plugin-specific state
• Use reducers instead of setState
• Change Lens plugins to close over their creation params (params which should include relevant observables)
• Remove side effects from addNewLayer, and react explicitly to layer creation rather than layer building
• Use selectors to grab nested state, rather than doing it ad-hoc everywhere
• Only have one new platform plugin (Lens) rather than one per Lens plugin
• Create a more structured, explicit initialization phase

Details

Request coordinator

We have a number of race conditions in Lens. Loading field existence info, data fetches as a response to route changes, etc. These would be trivially mitigated by usng a request coordination mechanism. If we are in the middle of fetching existing fields for a date range when the user changes the date range again, we wait for the previous fetch to complete before running the latest fetch. If N fetch requests are queued up while the first is in-flight, we only run the Nth when the first completes. The rest can be discarded.

Avoid use of useEffect

useEffect is a footgun.

The useEffect hook makes a component more than a function of its input. It becomes something like a process. It's stateful, tracking changes across function (render) invocations. It's effectful (performing a pull or push and usually both), and it is still a transform (transforming its properties into VDOM nodes). It inhibits reasoning about React's top-down dataflow.

In Lens, we set a bit of state, and React re-renders, and one (or more) effects fire off, and then they set a bit of state, which causes React to re-render, which might fire off even more effects, etc.

The existence of useEffect is a code-smell. It indicates a deficiency in our architecture which forced us to reach for a complex and brittle workaround.

Instead, we should either use observables in a proper data module and / or explicit event / effect mechanisms that live outside of the UI in a well-defined, tightly controlled place.

Avoid setState

Using setState encourages sprinkling state-transformation logic throughout our UI components. The UI components become concerned with both "what" and "how". Avoiding this requires a lot of discipline. If we used a reducer pattern or event dispatch pattern, the discipline would be enforced. UI components would simply say "what", and we'd have a tightly controled data layer that concerned itself with "how". We'd also get auditing for free. Right now, it's hard to answer, "Why did my state just change?" and even harder to answer, "What are the sequence of actions that got me into this state?" A reducer-based state management system makes this pretty easy, as it can log all actions and state transitions.

Move plugin registration to the root

It's not an editor frame concern. It's a Lens concern.

Move state to the root

The typically advised React architecture involves state at the root, and a top-down data flow. We've got state in several layers, and this complicates things like properly handling route changes (try going from an edit route to an empty Lens route in master), and it introduces some two-way dataflows (see onChange in the editor frame). Lens would be architecturally simplified if we stuck with a top-down dataflow.

Change Lens plugins to close over their creation params

Lens plugins are currently stateless. This has a lot of benefits. The main drawback, though, is that visualizations need to concern themselves with datasource implementatino details such as passing drag/drop context through. The lack of any context is also what requires us to rebuild the datasource public API during the render phase, rather than this being simply a subset of the datasource itself.

Create a lens$ observable

This would allow plugins that cared about it to react to changes to standard state (dateRange, drag context, etc), and others could safely ignore it. When combined with the plugin closure recommendation above, this would go a long way to eliminating the implementation-detail pass-through mentioned there.

Get rid of the effectful addNewLayer in the frame API

It's impossible to build new visualization states / layers without triggering side-effects and re-renders. This is because addNewLayer is the mechanism by which visualizations get new layer ids for their layer states.

This violates the principle of least surprise.

In this case, those the datasources don't care that you are building a new layer. They only care to know when a new layer is added to the system.

initialize(frame, state) {
  return (
    state || {
      // Here, we're just building up our new visualization state,
      // but thi has the side-effect of changing the datasource state.
      layerId: frame.addNewLayer(),
      accessor: generateId(),
    }
  );
},

There are a number of possible ways to fix this, but the smallest change is probably something like the following pseudo code:

const visualizationState = visualization.initialize(opts);
const visualizationLayerIds = visualization.getLayers(visualizationState).map(l => l.id);
const addedLayerIds = difference(datasourceLayerIds, visualizationLayerIds);
const removedLayerIds = difference(visualizationLayerIds, datasourceLayerIds);
let datasourceStates = removeDatasourceLayers(state.datasourceStates, removeddLayerIds);
datasourceStates = addDatasourceLayers(state.datasourceStates, addedLayerIds);

setState(s => ({
  ...s,
  datasourceStates,
  visualization: {
    id: visualization.id,
    state: visualizationState,
  },
}));

It's a minor tweak, but it keeps things non-effectful.

Use selectors

We have a number of places where we are accessing nested state in a way that breaks when state implementation-details change. For example:

const datasourceState = state.datasourceStates[datasourceId].state;

That line of code is duplicated in a number of files, and the same is true of logic that extracts other bits of nested state. If we ever change the nesting structure (as I'm doing in a current PR), a lot of code needs to change. We'd probably benefit from a selector module which hides the implementation detail of commonly selected state behind a function:

const datasourceState = selectDatasourceState(datasourceId, state);

Only have one new platform plugin

We really only need one new platform plugin right now: Lens. We have a bunch of somewhat convoluted code in our varous "plugins" because we've tried to make them new-platform ready. But we should solve that if and when we need to. More than likely, we'll only end up with two Lens plugins in the future: x-pack and oss. All visualizations and datasources could just live inside of those and register themselves with whatever registry we end up inventing.

Explicit initialization phase

Right now, initialization is done piece-meal through a series of useEffects. We need to get rid of useEffects, as mentioned above, but we should also not simply move initialization logic 1:1 into rxjs observables. Initialization should be predictable and written as a single, async initialization function:

// Pseudo code...
await initDatasources(savedDoc);
await initVisualization(savedDoc);
// etc..

[elasticmachine]

Pinging @elastic/kibana-app (Team:KibanaApp)

[flash1293]

This has a lot of overlap with #52792, I'm going to close this one