Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Embeddables Rebuild] Decouple other actions from Embeddable framework #175138

Closed
25 of 36 tasks
Tracked by #167429
ThomThomson opened this issue Jan 18, 2024 · 1 comment
Closed
25 of 36 tasks
Tracked by #167429
Labels
Feature:Embeddables Relating to the Embeddable system impact:critical This issue should be addressed immediately due to a critical level of impact on the product. loe:medium Medium Level of Effort project:embeddableRebuild Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas

Comments

@ThomThomson
Copy link
Contributor

ThomThomson commented Jan 18, 2024

Overview

@elastic/kibana-presentation is working on a large effort to migrate Embeddable decoupling from inheritance to composition. This will require migrating actions from the existing Embeddable framework to sets of composable interfaces that define an API's capabilities and an Action's requirements. An action is compatible with an API when the API's capabilities intersect with the Action's requirements.

Need a refresher on actions and triggers?

  1. Start kibana with yarn start --run-examples
  2. In the left menu, click Analytics -> Developer examples
  3. Click Actions & Triggers example and read through overview

#175262 migrates FILTER_BY_MAP_EXTENT action. Please follow this example to learn how to migrate each embeddable interaction.

Tasks

in #167890, the all Actions from the Dashboard plugin and the Presentation Panel were migrated to use the new framework, decoupling them from the legacy Embeddable system.

In the PR that migrated those actions, many were left untouched to avoid ballooning the number of reviewers and files changed. The following actions may still need to be converted:

ML

Preview Give feedback

Security

Preview Give feedback

Observability

Preview Give feedback

Discover

Preview Give feedback

Visualizations

Preview Give feedback

Lens

Preview Give feedback

Presentation

Preview Give feedback

Drilldowns

Preview Give feedback

Misc

Preview Give feedback

This list is in progress and will need to be expanded

@botelastic botelastic bot added the needs-team Issues missing a team label label Jan 18, 2024
@ThomThomson ThomThomson added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas loe:medium Medium Level of Effort impact:critical This issue should be addressed immediately due to a critical level of impact on the product. Feature:Embeddables Relating to the Embeddable system labels Jan 18, 2024
@botelastic botelastic bot removed the needs-team Issues missing a team label label Jan 18, 2024
@nreese nreese changed the title [Embeddables Rebuild] Decouple other actions from Embeddables [Embeddables Rebuild] Decouple other actions from Embeddable framework Jan 22, 2024
nreese added a commit that referenced this issue Jan 25, 2024
…mework (#175262)

Part of #175138 and prerequisite
for #174960

PR decouples FILTER_BY_MAP_EXTENT action from Embeddable framework by
migrating to sets of composable interfaces.

### Inheritance to composition

Replace action context type with a type definition that is as narrow as
possible. How do you know which properties are needed? Look through the
action and find all usages of `embeddabe`. Add each usage to the
narrowed type definition.
```
// original action context type.
// Notice how type requires an Embeddable instance 
// when only a few properties from the embeddable are needed
export interface FilterByMapExtentActionContext {
  embeddable: Embeddable<FilterByMapExtentInput>;
}

// new action context type. 
// Notice how type explicitly states what is used and 
// is no longer tied to an Embeddable (even though an Embeddable will pass the type guard)
// Another point to highlight is use of 'Partial'. This indicates that API interfaces are not required
export type FilterByMapExtentActionApi = HasType &
  Partial<HasDisableTriggers & HasParentApi<HasType> & HasVisualizeConfig>;

// This is the new action context rewritten to inline types like HasType to make it clearer 
export type FilterByMapExtentActionApi = {
  type: string;
  disableTriggers?: boolean;
  parentApi?: {
    type: string
  };
  getVis?: () => Vis<VisParam>;
}
```

### Migrating embeddable.parent?.type
Action uses `embeddable.parent?.type` to customize the display name. For
dashboards the display name would be "Filter dashboards by map bounds",
otherwise the display name would be "Filter page by map bounds".

To access `parentApi`, we must:

1) `HasParentApi` interface and `apiHasParentApi` type guard already
exist at
`packages/presentation/presentation_publishing/interfaces/has_parent_api.ts`,
so we can just use those.
2) No compatibility changes are required since Embeddable already
exposes `parentApi`.
3) Add `Partial<HasParentApi<HasType>>` to action compatibility
definition. Using `Partial` because accessing `parentApi` is not
required.
4) Access value like `api.parentApi?.type`

### Migrating embeddable.getInput().disableTriggers
Action fails compatibility check when
`embeddable.getInput().disableTriggers` returns `true`.

To access `disableTriggers`, we must:

1) Define `HasDisableTriggers` interface and `apiHasDisableTriggers`
type guard in the package
`packages/presentation/presentation_publishing`. Adding interface and
type guard to generic package folder since `input.disableTriggers` is
not tied to any specific Embeddable implementation. *Naming convention:
Use prefix `has` since `disableTriggers` is static. Use prefix
`publishes` for dynamic values since value will be accesses via a
subscription.*
2) Add compatibility changes to
`src/plugins/embeddable/public/lib/embeddables/embeddable.tsx` so that
an Embeddable instance exposes `disableTriggers`.
3) Add `Partial<HasDisableTriggers>` to action compatibility definition.
Using `Partial` because accessing `disableTriggers` is not required.
4) Access value like `api.disableTriggers`

### Migrating embeddable.getVis()
Action uses `embeddable.getVis()` to pass compatibility check when
visualize embeddable is a region_map or tile_map

To access `getVis`, we must:

1) Define `HasVisualizeConfig` interface and `apiHasVisualizeConfig`
type guard in `src/plugins/visualizations/public/embeddable/interfaces`.
Adding interface to the visualize plugin since `getVis` is only
available for Visualization Embeddable.
2) No compatibility changes are required since Visualization Embeddable
already exposes `getVis` method.
3) Add `Partial<HasVisualizeConfig>` to action compatibility definition.
Using `Partial` because accessing `getVis` is not required.
4) Access value like `api.getVis()`

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
nreese added a commit that referenced this issue Jan 29, 2024
…#175642)

Part of #175138 and prerequisite
for #174960

PR decouples SYNCHRONIZE_MOVEMENT_ACTION from Embeddable framework by
migrating to sets of composable interfaces.

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Devon Thomson <devon.thomson@elastic.co>
Heenawter added a commit that referenced this issue Jan 31, 2024
… framework (#175928)

Part of #175138

## Summary

This PR decouples the `DEPRECATION_BADGE` action (which is used
specifically for the legacy input controls panel) from Embeddable
framework by migrating to sets of composable interfaces.

**Testing:**
Since the legacy input control vis is deprecated and doesn't show up in
the "Add panel" menu anymore, you need to navigate to `<base
url>/app/visualize#/create?type=input_control_vis` in order to create
this panel type for testing.

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
nreese added a commit that referenced this issue Feb 5, 2024
Part of #175138 and prerequisite
for #174960

PR decouples Url drilldown action from Embeddable framework by migrating
to sets of composable interfaces.

### test instructions
1. Create panel and add "Url drilldown"
2. Verify `context` variables are populated with the same values when
they were grabbed from embeddable.input and embeddable.output
<img width="600" alt="Screenshot 2024-01-31 at 2 11 20 PM"
src="https://github.com/elastic/kibana/assets/373691/150f7a61-911f-4fb6-bf85-5c0481865e4b">

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
fkanout pushed a commit to fkanout/kibana that referenced this issue Feb 7, 2024
Part of elastic#175138 and prerequisite
for elastic#174960

PR decouples Url drilldown action from Embeddable framework by migrating
to sets of composable interfaces.

### test instructions
1. Create panel and add "Url drilldown"
2. Verify `context` variables are populated with the same values when
they were grabbed from embeddable.input and embeddable.output
<img width="600" alt="Screenshot 2024-01-31 at 2 11 20 PM"
src="https://github.com/elastic/kibana/assets/373691/150f7a61-911f-4fb6-bf85-5c0481865e4b">

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
nreese added a commit that referenced this issue Feb 12, 2024
part of #175138

PR decouples FlyoutCreateDrilldownAction, FlyoutEditDrilldownAction, and
EmbeddableToDashboardDrilldown from Embeddable framework.

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this issue Feb 15, 2024
…mework (elastic#175262)

Part of elastic#175138 and prerequisite
for elastic#174960

PR decouples FILTER_BY_MAP_EXTENT action from Embeddable framework by
migrating to sets of composable interfaces.

### Inheritance to composition

Replace action context type with a type definition that is as narrow as
possible. How do you know which properties are needed? Look through the
action and find all usages of `embeddabe`. Add each usage to the
narrowed type definition.
```
// original action context type.
// Notice how type requires an Embeddable instance 
// when only a few properties from the embeddable are needed
export interface FilterByMapExtentActionContext {
  embeddable: Embeddable<FilterByMapExtentInput>;
}

// new action context type. 
// Notice how type explicitly states what is used and 
// is no longer tied to an Embeddable (even though an Embeddable will pass the type guard)
// Another point to highlight is use of 'Partial'. This indicates that API interfaces are not required
export type FilterByMapExtentActionApi = HasType &
  Partial<HasDisableTriggers & HasParentApi<HasType> & HasVisualizeConfig>;

// This is the new action context rewritten to inline types like HasType to make it clearer 
export type FilterByMapExtentActionApi = {
  type: string;
  disableTriggers?: boolean;
  parentApi?: {
    type: string
  };
  getVis?: () => Vis<VisParam>;
}
```

### Migrating embeddable.parent?.type
Action uses `embeddable.parent?.type` to customize the display name. For
dashboards the display name would be "Filter dashboards by map bounds",
otherwise the display name would be "Filter page by map bounds".

To access `parentApi`, we must:

1) `HasParentApi` interface and `apiHasParentApi` type guard already
exist at
`packages/presentation/presentation_publishing/interfaces/has_parent_api.ts`,
so we can just use those.
2) No compatibility changes are required since Embeddable already
exposes `parentApi`.
3) Add `Partial<HasParentApi<HasType>>` to action compatibility
definition. Using `Partial` because accessing `parentApi` is not
required.
4) Access value like `api.parentApi?.type`

### Migrating embeddable.getInput().disableTriggers
Action fails compatibility check when
`embeddable.getInput().disableTriggers` returns `true`.

To access `disableTriggers`, we must:

1) Define `HasDisableTriggers` interface and `apiHasDisableTriggers`
type guard in the package
`packages/presentation/presentation_publishing`. Adding interface and
type guard to generic package folder since `input.disableTriggers` is
not tied to any specific Embeddable implementation. *Naming convention:
Use prefix `has` since `disableTriggers` is static. Use prefix
`publishes` for dynamic values since value will be accesses via a
subscription.*
2) Add compatibility changes to
`src/plugins/embeddable/public/lib/embeddables/embeddable.tsx` so that
an Embeddable instance exposes `disableTriggers`.
3) Add `Partial<HasDisableTriggers>` to action compatibility definition.
Using `Partial` because accessing `disableTriggers` is not required.
4) Access value like `api.disableTriggers`

### Migrating embeddable.getVis()
Action uses `embeddable.getVis()` to pass compatibility check when
visualize embeddable is a region_map or tile_map

To access `getVis`, we must:

1) Define `HasVisualizeConfig` interface and `apiHasVisualizeConfig`
type guard in `src/plugins/visualizations/public/embeddable/interfaces`.
Adding interface to the visualize plugin since `getVis` is only
available for Visualization Embeddable.
2) No compatibility changes are required since Visualization Embeddable
already exposes `getVis` method.
3) Add `Partial<HasVisualizeConfig>` to action compatibility definition.
Using `Partial` because accessing `getVis` is not required.
4) Access value like `api.getVis()`

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this issue Feb 15, 2024
…elastic#175642)

Part of elastic#175138 and prerequisite
for elastic#174960

PR decouples SYNCHRONIZE_MOVEMENT_ACTION from Embeddable framework by
migrating to sets of composable interfaces.

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Devon Thomson <devon.thomson@elastic.co>
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this issue Feb 15, 2024
… framework (elastic#175928)

Part of elastic#175138

## Summary

This PR decouples the `DEPRECATION_BADGE` action (which is used
specifically for the legacy input controls panel) from Embeddable
framework by migrating to sets of composable interfaces.

**Testing:**
Since the legacy input control vis is deprecated and doesn't show up in
the "Add panel" menu anymore, you need to navigate to `<base
url>/app/visualize#/create?type=input_control_vis` in order to create
this panel type for testing.

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this issue Feb 15, 2024
Part of elastic#175138 and prerequisite
for elastic#174960

PR decouples Url drilldown action from Embeddable framework by migrating
to sets of composable interfaces.

### test instructions
1. Create panel and add "Url drilldown"
2. Verify `context` variables are populated with the same values when
they were grabbed from embeddable.input and embeddable.output
<img width="600" alt="Screenshot 2024-01-31 at 2 11 20 PM"
src="https://github.com/elastic/kibana/assets/373691/150f7a61-911f-4fb6-bf85-5c0481865e4b">

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
CoenWarmer pushed a commit to CoenWarmer/kibana that referenced this issue Feb 15, 2024
part of elastic#175138

PR decouples FlyoutCreateDrilldownAction, FlyoutEditDrilldownAction, and
EmbeddableToDashboardDrilldown from Embeddable framework.

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
fkanout pushed a commit to fkanout/kibana that referenced this issue Mar 4, 2024
…elastic#175642)

Part of elastic#175138 and prerequisite
for elastic#174960

PR decouples SYNCHRONIZE_MOVEMENT_ACTION from Embeddable framework by
migrating to sets of composable interfaces.

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Devon Thomson <devon.thomson@elastic.co>
fkanout pushed a commit to fkanout/kibana that referenced this issue Mar 4, 2024
… framework (elastic#175928)

Part of elastic#175138

## Summary

This PR decouples the `DEPRECATION_BADGE` action (which is used
specifically for the legacy input controls panel) from Embeddable
framework by migrating to sets of composable interfaces.

**Testing:**
Since the legacy input control vis is deprecated and doesn't show up in
the "Add panel" menu anymore, you need to navigate to `<base
url>/app/visualize#/create?type=input_control_vis` in order to create
this panel type for testing.

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
fkanout pushed a commit to fkanout/kibana that referenced this issue Mar 4, 2024
Part of elastic#175138 and prerequisite
for elastic#174960

PR decouples Url drilldown action from Embeddable framework by migrating
to sets of composable interfaces.

### test instructions
1. Create panel and add "Url drilldown"
2. Verify `context` variables are populated with the same values when
they were grabbed from embeddable.input and embeddable.output
<img width="600" alt="Screenshot 2024-01-31 at 2 11 20 PM"
src="https://github.com/elastic/kibana/assets/373691/150f7a61-911f-4fb6-bf85-5c0481865e4b">

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
fkanout pushed a commit to fkanout/kibana that referenced this issue Mar 4, 2024
part of elastic#175138

PR decouples FlyoutCreateDrilldownAction, FlyoutEditDrilldownAction, and
EmbeddableToDashboardDrilldown from Embeddable framework.

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Heenawter added a commit that referenced this issue Mar 6, 2024
…mework (#176953)

Part of #175138

## Summary

This PR decouples all Discover-owned actions (`ViewSavedSearchAction`,
`ExploreDataChartAction`, and `ExploreDataContextMenuAction`) from the
Embeddable framework.

> [!NOTE]
> In order to test the latter two actions, you must add the following to
your `kibana.dev.yml`:
>```yml
> xpack.discoverEnhanced.actions.exploreDataInContextMenu.enabled: true
> xpack.discoverEnhanced.actions.exploreDataInChart.enabled: true
> ```

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios


### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Heenawter added a commit that referenced this issue Mar 11, 2024
…framework (#178048)

Part of #175138

## Summary

This PR decouples the `EditInLensAction` from the Embeddable framework.

### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Stratoula Kalafateli <efstratia.kalafateli@elastic.co>
nreese added a commit that referenced this issue Mar 11, 2024
part of #175138

PR decouples `createMLADJobAction` action from Embeddable framework.
This means that instead of reading values from `embeddable.getInput()`,
values are read from presentation publishing interfaces. Existing
embeddables expose both `Embeddable` and `presentation publishing
interfaces` so they work with both. In the future, as embeddables get
refactored to the new embeddable system, then they will only expose
`presentation publishing interfaces`. Migrating away from old interfaces
so that refactoring embeddables can start.

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
nickpeihl added a commit that referenced this issue Mar 19, 2024
… framework (#177888)

## Summary

Decouples the "Explore data in Discover" panel action and the "Open in
Discover" drilldown from the legacy embeddable framework.

part of #175138
nreese added a commit that referenced this issue Jul 8, 2024
Part of #175138

PR decouples "Add to case" action from legacy embeddable framework. PR
also cleans up page load bundle size by moving `isCompatible` and
`execute` implemenations behind async imports.

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@nreese
Copy link
Contributor

nreese commented Aug 23, 2024

Closing, all actions have been converted.

@nreese nreese closed this as completed Aug 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Embeddables Relating to the Embeddable system impact:critical This issue should be addressed immediately due to a critical level of impact on the product. loe:medium Medium Level of Effort project:embeddableRebuild Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas
Projects
None yet
Development

No branches or pull requests

2 participants