[UIActions] Embeddable and data plugin triggers/actions race condition

[Dosant]

Kibana version: master, 7.x

Steps to reproduce:

This is hard to catch race conditions. Appeared in builds.

This would happen when data plugin setup is called before embeddable plugin setup:

1. Embeddable registers a trigger:

   kibana/src/plugins/embeddable/public/bootstrap.ts

   Line 27 in 4ee9603

   uiActions.registerTrigger(valueClickTrigger);

2. Data plugin attaches an action to a trigger:

   kibana/src/plugins/data/public/plugin.ts

   Lines 116 to 121 in 4ee9603

   	uiActions.addTriggerAction(
   	'VALUE_CLICK_TRIGGER',
   	createValueClickAction(() => ({
   	uiActions: startServices().plugins.uiActions,
   	}))
   	);

Data plugin and embeddable plugin do not have an explicit dependency on each other. So it could happen that the data plugin is loaded before the embeddable plugin and it is causing an error.

This is most likely a regression since #82791

possible solutions:

1. Try to move registering actions to a start phase in data plugin
2. Make data depend on embeddable?
3. move those triggers to data plugin?

[elasticmachine]

Pinging @elastic/kibana-app-services (Team:AppServices)

[Dosant]

Added a blocker level because I assume this theoretically could appear pretty often depending on network conditions.

[lukeelmers]

TLDR I think modifying the trigger context typings and going with option (3) (move to data plugin) is probably the right move here, but here are a few more thoughts:

1. Try to move registering actions to a start phase in data plugin

That would probably work, but doesn't resolve the issue of the implicit dependency we have here.

2. Make data depend on embeddable?

IIRC from #82791, I tried this and it created another circular dependency between data > embeddable > saved_objects_management > data

3. move those triggers to data plugin?

This could be an option, but TBH the source of a lot of issues I ran into relates to the way the actions/trigger contexts are typed. Many of these triggers have embeddable in their context and are mixing with other domains. For example value click has an embeddable, but also some data fields which would otherwise make sense to live in the data plugin:

kibana/src/plugins/embeddable/public/lib/triggers/triggers.ts

Lines 18 to 30 in 4ee9603

	export interface ValueClickContext<T extends IEmbeddable = IEmbeddable> {
	embeddable?: T;
	data: {
	data: Array<{
	table: Pick<Datatable, 'rows' | 'columns'>;
	column: number;
	row: number;
	value: any;
	}>;
	timeFieldName?: string;
	negate?: boolean;
	};
	}

This makes it really hard to relocate them because you either need to declare a dependency on embeddable wherever they live, or you need to fake it like I did here:

kibana/src/plugins/data/public/actions/value_click_action.ts

Lines 18 to 32 in 4ee9603

	export interface ValueClickContext {
	// Need to make this unknown to prevent circular dependencies.
	// Apps using this property will need to cast to `IEmbeddable`.
	embeddable?: unknown;
	data: {
	data: Array<{
	table: Pick<Datatable, 'rows' | 'columns'>;
	column: number;
	row: number;
	value: any;
	}>;
	timeFieldName?: string;
	negate?: boolean;
	};
	}

This requires downstream folks relying on this piece of context to cast to IEmbeddable, which is less convenient but also more explicit as they will have to then depend on embeddable to have things typed correctly. For that reason (explicitness), I prefer this approach... but take all of this with a grain of salt as I don't know the ui actions service very well 😉