Make Lens embeddable respect disableTriggers input

[timroes]

Any embeddable can have an disableTriggers: true set on their embeddable input. If this is set, it means, that the content of the embeddable should never execute any triggers (i.e. call the executeTriggerActions method). This will be used e.g. by Canvas to disable that filtering works in charts.

If an embeddable ignores this (as Lens currently does) you can still trigger e.g. the filter dialog for multiple filters in Canvas, even though it has no effect anymore when you click something in it:

The correct solution will be not to call the executeTriggerActions method inside charts if this flag was set to the embeddable. Now to the problems:

The chart (which has that code logic) is rendered using an expression renderer. That is only registered once, and we currently don't have a way of passing information (whether the disableTriggers flag was set) from a specific embeddable down to that renderer's render function.

I talked a bit with @stacey-gammon and so far the best solution we could still come up with is, actually making the embeddable and in that term maybe also executeTriggerActions a high-level concept on the handlers so we can access it in all renderers via the handlers.

This would still require the ExpressionExecutorService as the corresponding React components to accept the embeddable as an input.

I currently don't see any way without changing something in the expression plugin to get the information down into the render function.

@elastic/kibana-app-arch I think we need your input here (cc @lukeelmers since we just closed an issue with a similar discussion).

[streamich]

1. Maybe Lens embeddable could do what visualize_embeddable is doing, e.g use "events" on expression renderer handlers. Basically events are bubbled up to Lens embeddable, and Lens embeddable decides what to do with them.

2. Another approach, that probably does not need any modifications to Expressions service could be to pass in Lens embeddable into expression using expression execution context, currently that is not directly exposed though, but context.variables is exposed, for example, in this code one could do:

      <ExpressionRendererComponent
        variables={{embeddable: props.embeddable}}
        {...}
      />

And then in expression function access it:

  fn(data: LensMultiTable, args: XYArgs, { variables }) {
    return {
      type: 'render',
      as: 'lens_xy_chart_renderer',
      value: {
        data,
        args,
        embeddable: variables.embeddable,
      },
    };
  },

Maybe actually it is better to use the variables instead of the context directly.

3. We could also add some modifications to Expressions service.
   1. We could expose extending execution context from React component, e.g <ExpressionRendererComponent executionContext={{embeddable: this}}>, instead of using variables.
   2. Like suggested by OP (if I understood correctly), we could also add ability to extend handlers of expression renderers (not to be confused with handlers/context of expression functions).

[timroes]

Let me elaborate on the approaches here a bit:

1. I am not a huge fan of that idea, for two reasons mainly: First, if put the code that triggers uiActions into the embeddable instead of into the charts itself, we'll need to link up uiActions in all the places we run without embeddables (e.g. in the editor). Meaning we basically need to have the code to execute triggers in multiple places, why imho it should rather just be triggered by the charts. Also this is currently one of the worst typed ways, since due to usage of any around that API we have zero type safeness in that place.

2. Passing in via variables could work as a short term solution, but given that variables is something that's meant to be used inside the expressions themselves, I don't think we should have this in for a long time?

3. That would be kind of my preferred way and suggestion for now. Making the embeddable an optional parameter to pass into the executionContext and passing it onto the renderer handlers (not the expression functions, I don't think they would need that, or at least I currently can't come up with any reasonable example). Besides that I would also push the executeActionTriggers directly into the renderer handlers, since we can expect most charts to need emitting events at some point and I think it makes it easier if this function will be passed down. Ideally I would link this function that's passed into the handler, directly up to the disableTriggers. Meaning if the user specified an embeddable in the executionContext that did have disableTriggers set to true, the function passed into the renderer handlers will basically be a noop function. That way, most embeddables wouldn't even need to handle that flag themselves, and it's easier to have al embeddables follow this rules.

[ppisljar]

making embeddable high level concept on handlers is in my opinion the worst possible option, as that would mean renderers only work inside embeddables ... think about the consequence for canvas.

the concept of renderers is very simple, they get dom element, config(everything they need to render, including data) and handlers, which allow them to communicate with outside world (done method to signal when rendering is complete, destroy method to allow registering cleanup, event method to signal events inside renderers (like clicks etc) and update to request re-render

if there is anything that should always be passed to specific renderer, it should be made part of its config

if there is anything that should be passed down to every handler we should really revisit if that is really necessarily as that affects everything using them and we must make sure that information makes sense in all the contexts.

i think from this perspective embeddable should be off the list. if we would still consider moving uiActions on the handlers (to replace the event method there) i think this will greatly limit the flexibility, as parent component (requesting the rendering) suddenly has no way to intercept events and react to them, chart talks directly with uiActions (again, think about canvas)

[timroes]

I just synced with Peter and that is the outcome:

We'll use option #1 above: We use the event method on handlers to emit an event that will then be converted inside the embeddable (and in our case also inside the editor) into uiActions that will be emitted.

App Arch will prioritize fixing the following two things, before we can switch:

• Make sure that the event function is properly types. Since we (at least for now) know all the events coming through, we'll create full typings for the event parameter so we can in the render functions and inside the embeddable and other consuming places have full type safeness for them.
• Make sure that we can get access to the events using the ReactExpressionRenderer. Therefore it will get a new property onEvent which will be called every time an event is emitted from the underlaying ExpressionService.

[flash1293]

Resolved by #65675 Lens uses the same event types as visualize to pass the filter context up to the embeddable.