[Lens] Cleanup and architectural adjustments

[chrisdavies]

This is a list of technical debt or changes that I think might be worth considering.

Summary

• Move state to the root
• As much as possible, remove calls to useEffect
• Use hooks as infrequently as is practical
• Adopt a more disciplined approach to state (maybe action / reducer pattern)
• Clean up suggestion logic
• Make initialization non-effectful
• UX: implement a visual representation of the underlying query / data

Move state to the root

This is generally the right default for React apps, and we got this wrong. We have pretty significant chunks of state living at different levels in the component heirarchy. This makes it non-trivial to do things like reset the state when changing between URL routes. This is also the reason we have an inefficient "isDirty" mechanism which causes circular state changes.

As a rule in my past projects (Lens now included), I regretted not placing state at the root. Inevitably, I'd think the state belonged in a component at depth X, but then some requiremnet would come along that required coordinating that state at depth X - 2 or whatever. This happens all the time. Keeping state at the root is generally the right default.

Remove useEffect and any other hooks where practical

It's almost a tautology to say that effectful functions are harder to reason about than pure functions. That's why Haskell, Elm, and the like spend so much effort in restricting effects. We have useEffect scattered throughout various components, and in almost every case, it is because of a defect in our state management layer. The end-result is data-flow that is hard to reason about.

I change property X, and it causes an effect to fire, which changes property Y, which causes an effect to fire which changes property Z. What we should instead do is ask, "What is the action or intent which necessitates this effect?" Then, make that action explicit.

We should consider the existence of useEffect to be a code smell, or at least a yellow flag.

For example, we have some logic which attempts to ensure that visualizations don't initialize until the datasources have initialized. This is done using an effect, that observes when all datasources are no longer in the isLoading state, or something. Instead, we could have an explicit initialization phase which would make this entire process linear and direct.

We should consider moving our state out of React altogether and put it into explicit state modules (per plugin, and one for core Lens state). We could use RxJs to allow plugins to react to state changes. This sounds like it just pushes the problem elsewhere, but it doesn't.

I've prototyped this RxJs change, and it was in fact cleaner. It also improves testing.

First, useEffect is harder to test. In order to test the effect, you need to initialize components, and deal with the React component lifecycle. That makes the tests more complex and brittle. Often, unrelated changes to the component can cause test failures, especially when dealing with large components with many effects chained in them. When using an RxJs layer, tests became a lot simpler: given observable x and y, z should equal whatever.

React components also become easier to test and reason about. With state changes and effects living in explicit, data modules, most React components become a pure function of their inputs.

Use a disciplined approach to state

We use setState just about everywhere. I thought this was more direct and simple, but I've changed my mind. setState combines what with how in a way that action (what) and reducers (how) don't. Without discipline, the use of setState tends to mean that you have state manipulation logic scattered randomly throughout a great many components. Again, testing this is tricky. In order to test your state change logic, you need to wire up a bunch of unrelated React stuff.

Testing actions and reducers is much easier. You have pure components, and simple tests that simply say: "When the user clicks foo, the bar action fires", and somewhere else, you have a unit test that says: "When the bar action fires, the state should transition from X to Y."

The other advantage to the action / reducer pattern is tracing. It's pretty trivial to answer the question, "How did I get into this state?" You can simply look at what actions were fired off, how the state changed in response to each action, and then it's pretty trivial to find all call-sites that could trigger the offending action. With setState, this process is much less straightforward. State might be changed by some long-running async process, or by an effect, or any number of random components. It's pretty hard to diagnose.

This is less clearly developed, but I suspect we'll see long-term systematic advantages to a more discilplined approach to state.

What I prototyped was a system in which:

• Lens had private core state, and public, observable state (date range, filter, etc)
• Plugins defined their own state, reducer, and actions
• When a plugin fired off an action, it was scoped only to that plugin's reducer
• Two plugins could fire off actions with the same name, but not run into any conflicts
• Public / Lens-wide actions were prefixed with a '$' (e.g. dispatch({ type: '$CLEAR_DATE_FILTER' }))
• Plugin-specific / local actions could not begin with a '$'
• Plugins could subscribe to the public core state via RxJs observables
• Plugin reducers could respond to public actions
• None of this used React or Redux, just RxJs and some simple functions

Clean up suggestion logic

I don't have a lot to add here, but it's worth attempting. It's a pretty complex and relatively brittle bit of code.

In my (super messy) PoC for a different interaction model, plugins defined a data template. For example, a line chart might have a data template like this:

const xyTemplate = [
  { id: 'x', label: 'x', isRequired: true, isCategorical: true },
  { id: 'y', label: 'y', isRequired: true, type: 'number' },
  { id: 'color', label: 'color', isCategorical: true },
  { id: 'size', label: 'size', type: 'number' },
];

This is not comprehensive, and probably has design flaws, but this paves the way for:

• A way for datasources to suggest something relevant to a given data template, rather than generating all possible table shapes
• Simpler visualizations due to a systematic assignment of data columns to chart dimensions (no visualization-specific logic required)

For example, here's the entirety of the line chart plugin for the PoC:

const lineVis = {
  id: 'line',

  dimensionTemplate: xyTemplate,

  Vis({ data, categories }) {
    const config = {
      data,
      type: 'line',
      x: categories.x?.tag,
      y: categories.y?.tag,
      color: categories.color?.tag,
      size: categories.size?.tag,
      plugins: [
        tauCharts.api.plugins.get('tooltip')(),
        tauCharts.api.plugins.get('legend')(),
      ],
      guide: {
        interpolate: 'smooth-keep-extremum',
        x: { label: { text: dimensionName(categories.x?.dimension) } },
        y: { label: { text: dimensionName(categories.y?.dimension) } },
        color: { label: { text: dimensionName(categories.color?.dimension) } },
        size: { label: { text: dimensionName(categories.size?.dimension) } },
      },
    };

    return <TauChart config={config} />;
  },
};

Everything else is handled by the core engine, which makes writing new visualizations pretty fast and trivial-- something which is untrue of Lens. With Lens, to write a visualization, among other things, you need to:

• Register various expression functions
• Understand the datasource interface
• Write your own config panel UI

This also has the nice property of no longer making visualizations responsible for generateId or anything of the sort. The ids are really only relevant to the visualization. The datasources themselves have their own internal ids for their columns, and the two things are matched up via core Lens coordination logic.

Make initialization non-effectful

Right now, when you dig into Lens visualization initialization (or really any part of the init logic), you see a lot of oddities. For example, visualizations generally call frame.newLayer or whatever it's called. This in turn calls the datasource(s), and that performs a setState. So, you have what appears to be a pure / clean initialize function for your visualization, but what's actually happening is incidental changes to React state.

With a bit of thought (and probably with a proper state management mechanism), this could be tidied up.

UX for seeing the query / data structure

See the interaction model PoC.

[elasticmachine]

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

[flash1293]

See also #52792

[flash1293]

Tracking this in #89690 now