[DISCUSS] Change ui_actions interface to more directly express dependencies

[mattkime]

This is in follow up to #60168 #60202 - tldr; ui_actions allows triggers and actions to be registered from different plugins to ui_actions but this creates a dependency not expressed through the plugin contracts. Tests became flakey due to indeterminate plugin loading order.

Proposal:

Instead of registering triggers and actions in a central registry, triggers would be created and made available via plugin contracts.

ui_actions service

Before	After
registerTrigger	new Trigger()
getTrigger	n/a
registerAction	trigger.registerAction
getAction	trigger.getAction
attachAction	n/a
detachAction	trigger.detachAction
getTriggerActions	trigger.getActions
getTriggerCompatibleActions	getCompatibleActions
fork	UNSURE

Part of #71854

[elasticmachine]

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

[stacey-gammon]

Nice, I like it.

One thing to consider is that I think most triggers should be created and exposed from a central place, like the uiActions plugin. We don't want two different solutions creating the same trigger. Imagine like an "IP_ADDRESS_TRIGGER", that SIEM could add actions to, and Maps. Who should register the trigger? SIEM, Maps, or the central uiActions plugin? We should probably have solutions look first to see if a trigger exists before creating one themselves, because otherwise it'll be difficult for Maps to think, oh let me see if an "IP ADDRESS" trigger exists in SIEM before I create one myself. Or if SIEM app is then disabled, ip address trigger should still work.

So I think we should encourage anyone who wants to create their own trigger to first consider having the app arch team create it so it's central and all solutions can attach their actions to it. Unless it's something very obviously explicit to one app or another (like if the trigger context is an ML job or something - that would mean nothing without the ML app actually existing).

If we do opt to have most triggers as part of a single plugin, then the flow would be something like:

uiActions/public/plugin.ts:

setup() {
  return {
    getApplyFilterTrigger(),
    getGeoLocationTrigger(),
    getIpAddressTrigger(),
    getValueClickTrigger(),
    ...
  }
}

lens/public/../render_xy_chart.ts

uiActions.getApplyFilterTrigger().executeActions({ context });

[mattkime]

thanks for the feedback @stacey-gammon

(I keep rewriting this)

After mulling over the case of registering the same trigger from multiple places I'm thinking about this a lot differently.

I like what you're proposing - returning the individual trigger interfaces by name. No need to look items up by id.

[streamich]

@mattkime I like this change, but I think we should only change these methods

Before	After
attachAction	trigger.attachAction
detachAction	trigger.detachAction
getTriggerActions	trigger.getActions
getTriggerCompatibleActions	trigger.getCompatibleActions

Currently, I don't see why other methods should be changed, but happy to learn.

---

@stacey-gammon What would be an advantage of

uiActions.getApplyFilterTrigger()

over current

uiActions.getTrigger('APPLY_FILTER_TRIGGER')

[mattkime]

What would be an advantage of

I might not be understanding the benefit of looking triggers up by key.

[stacey-gammon]

@stacey-gammon What would be an advantage of
uiActions.getApplyFilterTrigger()
over current
uiActions.getTrigger('APPLY_FILTER_TRIGGER')

That example, none. The example where it does make a difference is this one:
The benefit of this:

maps.getMapSpecificTrigger()

vs

 uiActions.getTrigger('MAP_SPECIFIC_TRIGGER')

is just that it forces the user to not forget to include map as a plugin dependency.

[Dosant]

If we do opt to have most triggers as part of a single plugin, then the flow would be something like:

uiActions/public/plugin.ts:

setup() {
  return {
    getApplyFilterTrigger(),
    getGeoLocationTrigger(),
    getIpAddressTrigger(),
    getValueClickTrigger(),
    ...
  }
}
lens/public/../render_xy_chart.ts

uiActions.getApplyFilterTrigger().executeActions({ context });

Does this mean we won't need global trigger / action / context types mapping?

[streamich]

Maybe we could do

setup() {
  return {
    triggers: {
      applyFilter,
      geoLocation,
      valueClick,
      rangeBrush,
      // ...
    }
  }
}

[mattkime]

Does this mean we won't need global trigger / action / context types mapping?

I think types would be simplified a bit but I'm not fully certain.

Maybe we could do ...

LGTM

---

@streamich pointed out that the trigger registry is needed for dynamic action storage. We can keep it and look up by id while referencing triggers directly in other places.

[stacey-gammon]

Does this mean we won't need global trigger / action / context types mapping?

Yea, I think so. However... I'm a bit worried about this conflicting with the "enhancements" pattern. I've written up my thoughts on this here: #62815

[mattkime]

@stacey-gammon Can you specify how this would conflict? I might be missing something.

[stacey-gammon]

If we ever wanted to follow the enhancements pattern as outlined in that issue (I am working on an example plugin to make this pattern clearer), then the object you are registering is not the same object you would retrieve off the registry, which makes me wonder if those types id type mappings would come in handy.

e.g.

  FooActionPlugin {
    setup(core, uiActions) {
      uiActions.register(actionDefinition);
    }

    start(core, uiActions) {
      return {
        // Will type id mappings be useful here?... more I think about it, no they won't because 
        // this is the same plugin that would be setting the id -> type mapping.
        getFooAction: () => uiActions.get(FOO_ACTION) as FooAction;
      }
    }
  }

Proceed as you wish, I just have this sneaking feeling I might be missing something that makes these id -> type mappings useful. But it's just a feeling so don't stop the refactor on that account.

Just make sure that the types will still complain when you do
uiActions.attach(actionNeedsFooContext, triggerEmitsBarContext)

[mattkime]

The current version of the enhancement pattern shows that we should be able to factor out the majority of id usage aside from loading of saved objects. This should simplify the type expressions as well. Getter functions are necessary since the plugin providing the enhancement may execute setup after items have registered.

Instead of doing a bit by bit analysis of the existing api I'd rather just say 'apply the enhancement pattern' since I might miss some wrinkles.

[vadimkibana]

Thank you for contributing to this issue, however, we are closing this issue due to inactivity as part of a backlog grooming effort. If you believe this feature/bug should still be considered, please reopen with a comment.