-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[embeddable] decouple FILTER_BY_MAP_EXTENT action from Embeddable framework #175262
Conversation
/ci |
Pinging @elastic/kibana-presentation (Team:Presentation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Visualization changes work for me!
@elasticmachine merge upstream |
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
async chunk count
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review only, LGTM! This is a great example of how to convert an action. Also, great work on the step-by-step instructions on the PR description.
…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>
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.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:HasParentApi
interface andapiHasParentApi
type guard already exist atpackages/presentation/presentation_publishing/interfaces/has_parent_api.ts
, so we can just use those.parentApi
.Partial<HasParentApi<HasType>>
to action compatibility definition. UsingPartial
because accessingparentApi
is not required.api.parentApi?.type
Migrating embeddable.getInput().disableTriggers
Action fails compatibility check when
embeddable.getInput().disableTriggers
returnstrue
.To access
disableTriggers
, we must:HasDisableTriggers
interface andapiHasDisableTriggers
type guard in the packagepackages/presentation/presentation_publishing
. Adding interface and type guard to generic package folder sinceinput.disableTriggers
is not tied to any specific Embeddable implementation. Naming convention: Use prefixhas
sincedisableTriggers
is static. Use prefixpublishes
for dynamic values since value will be accesses via a subscription.src/plugins/embeddable/public/lib/embeddables/embeddable.tsx
so that an Embeddable instance exposesdisableTriggers
.Partial<HasDisableTriggers>
to action compatibility definition. UsingPartial
because accessingdisableTriggers
is not required.api.disableTriggers
Migrating embeddable.getVis()
Action uses
embeddable.getVis()
to pass compatibility check when visualize embeddable is a region_map or tile_mapTo access
getVis
, we must:HasVisualizeConfig
interface andapiHasVisualizeConfig
type guard insrc/plugins/visualizations/public/embeddable/interfaces
. Adding interface to the visualize plugin sincegetVis
is only available for Visualization Embeddable.getVis
method.Partial<HasVisualizeConfig>
to action compatibility definition. UsingPartial
because accessinggetVis
is not required.api.getVis()