-
Notifications
You must be signed in to change notification settings - Fork 8.5k
Implementing ReferenceOrValueEmbeddable for visualize embeddable #76088
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
Changes from 7 commits
c014e35
dbafccb
7f2e4ad
3209dc2
9788589
2b6f075
e9dff6f
3c02a0e
f04c20f
d9e8bfb
11e878a
020c104
c04d0f7
db9adc0
1189f44
3e7668b
e645b98
5843645
cee9b81
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,6 @@ | |
| "version": "kibana", | ||
| "server": true, | ||
| "ui": true, | ||
| "requiredPlugins": ["data", "expressions", "uiActions", "embeddable", "usageCollection", "inspector"], | ||
| "requiredPlugins": ["data", "expressions", "uiActions", "embeddable", "usageCollection", "inspector", "dashboard"], | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we certain the only way to address this is to introduce a required dependency between This doesn't feel right to me, but I'm also not familiar enough with the I just wanted to raise the question before we move forward with merging this PR.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey Luke! This is a really good point. I made the decision to move The attributeService is an additional layer of abstraction on top of savedObjects. It provides a default implementation that embeddables can use to deal more easily with input that can be either 'by value' or 'by reference' . It also provides code for translating between the two types of input. The dashboard plugin is not its permanent home, however, it needs to stay somewhere until we find a better place for it. Do you have any thoughts on where it could live?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm, right i didn't notice this ...
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Attribute service is indeed embeddable specific, but it's also only used within the dashboard (at least for now). I don't think saved objects plugin is the best place for it, since it's more than just saved objects. If there are major concerns about this living inside the dashboard, I think a separate plugin is the best way to go. |
||
| "requiredBundles": ["kibanaUtils", "discover", "savedObjects"] | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,8 @@ import { | |
| Embeddable, | ||
| IContainer, | ||
| Adapters, | ||
| SavedObjectEmbeddableInput, | ||
| ReferenceOrValueEmbeddable, | ||
| } from '../../../../plugins/embeddable/public'; | ||
| import { | ||
| IExpressionLoaderParams, | ||
|
|
@@ -47,6 +49,11 @@ import { getExpressions, getUiActions } from '../services'; | |
| import { VIS_EVENT_TO_TRIGGER } from './events'; | ||
| import { VisualizeEmbeddableFactoryDeps } from './visualize_embeddable_factory'; | ||
| import { TriggerId } from '../../../ui_actions/public'; | ||
| import { SavedObjectAttributes } from '../../../../core/types'; | ||
| import { AttributeService } from '../../../dashboard/public'; | ||
| import { SavedVisualizationsLoader } from '../saved_visualizations'; | ||
| import { VisSavedObject } from '../types'; | ||
| import { OnSaveProps, SaveResult } from '../../../saved_objects/public'; | ||
|
|
||
| const getKeys = <T extends {}>(o: T): Array<keyof T> => Object.keys(o) as Array<keyof T>; | ||
|
|
||
|
|
@@ -75,9 +82,15 @@ export interface VisualizeOutput extends EmbeddableOutput { | |
| visTypeName: string; | ||
| } | ||
|
|
||
| export type VisualizeSavedObjectAttributes = SavedObjectAttributes & { title: string }; | ||
|
|
||
| export type VisualizeByValueInput = { attributes: VisualizeSavedObjectAttributes } & VisualizeInput; | ||
| export type VisualizeByReferenceInput = SavedObjectEmbeddableInput & VisualizeInput; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice looking input shapes! |
||
|
|
||
| type ExpressionLoader = InstanceType<ExpressionsStart['ExpressionLoader']>; | ||
|
|
||
| export class VisualizeEmbeddable extends Embeddable<VisualizeInput, VisualizeOutput> { | ||
| export class VisualizeEmbeddable extends Embeddable<VisualizeInput, VisualizeOutput> | ||
| implements ReferenceOrValueEmbeddable<VisualizeByValueInput, VisualizeByReferenceInput> { | ||
| private handler?: ExpressionLoader; | ||
| private timefilter: TimefilterContract; | ||
| private timeRange?: TimeRange; | ||
|
|
@@ -93,11 +106,22 @@ export class VisualizeEmbeddable extends Embeddable<VisualizeInput, VisualizeOut | |
| private abortController?: AbortController; | ||
| private readonly deps: VisualizeEmbeddableFactoryDeps; | ||
| private readonly inspectorAdapters?: Adapters; | ||
| private attributeService: AttributeService; | ||
|
|
||
| constructor( | ||
| timefilter: TimefilterContract, | ||
| { vis, editPath, editUrl, indexPatterns, editable, deps }: VisualizeEmbeddableConfiguration, | ||
| { | ||
| vis, | ||
| editPath, | ||
| editUrl, | ||
| indexPatterns, | ||
| editable, | ||
| deps, | ||
| visualizations, | ||
| }: VisualizeEmbeddableConfiguration, | ||
| initialInput: VisualizeInput, | ||
| attributeService: AttributeService, | ||
| savedVisualizationsLoader?: SavedVisualizationsLoader, | ||
| parent?: IContainer | ||
| ) { | ||
| super( | ||
|
|
@@ -118,6 +142,8 @@ export class VisualizeEmbeddable extends Embeddable<VisualizeInput, VisualizeOut | |
| this.vis = vis; | ||
| this.vis.uiState.on('change', this.uiStateChangeHandler); | ||
| this.vis.uiState.on('reload', this.reload); | ||
| this.attributeService = attributeService; | ||
| this.savedVisualizationsLoader = savedVisualizationsLoader; | ||
|
|
||
| this.autoRefreshFetchSubscription = timefilter | ||
| .getAutoRefreshFetch$() | ||
|
|
@@ -381,4 +407,43 @@ export class VisualizeEmbeddable extends Embeddable<VisualizeInput, VisualizeOut | |
| public supportedTriggers(): TriggerId[] { | ||
| return this.vis.type.getSupportedTriggers?.() ?? []; | ||
| } | ||
|
|
||
| inputIsRefType = (input: VisualizeInput): input is VisualizeByReferenceInput => { | ||
| return this.attributeService.inputIsRefType(input); | ||
| }; | ||
|
|
||
| getInputAsValueType = async (): Promise<VisualizeByValueInput> => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not use the attribute service here as well? The implementation is quite simple, but it could be more consistent that way.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried, but it's honestly too much hassle. I'd need a |
||
| const input = { | ||
| savedVis: this.vis.serialize(), | ||
| }; | ||
| delete input.savedVis.id; | ||
| return input; | ||
| }; | ||
|
|
||
| getInputAsRefType = async (): Promise<VisualizeByReferenceInput> => { | ||
| return new Promise<RefType>((resolve, reject) => { | ||
| const onSave = async (props: OnSaveProps): Promise<SaveResult> => { | ||
majagrubic marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| try { | ||
| const savedVis: VisSavedObject = await this.savedVisualizationsLoader.get({}); | ||
| const saveOptions = { | ||
| confirmOverwrite: false, | ||
| returnToOrigin: true, | ||
| }; | ||
| savedVis.title = props.newTitle; | ||
| savedVis.copyOnSave = props.newCopyOnSave || false; | ||
| savedVis.description = props.newDescription || ''; | ||
| savedVis.searchSourceFields = this.vis?.data.searchSource?.getSerializedFields(); | ||
| savedVis.visState = this.vis.serialize(); | ||
| savedVis.uiStateJSON = this.vis.uiState.toString(); | ||
| const id = await savedVis.save(saveOptions); | ||
| resolve({ savedObjectId: id }); | ||
| return { id }; | ||
| } catch (error) { | ||
| reject(error); | ||
| return { error }; | ||
| } | ||
| }; | ||
| this.attributeService.showSaveModal(onSave, '', 'visualization'); | ||
| }); | ||
| }; | ||
| } | ||
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.
I'm not quite sure why this change is required, why the bind, and why a ternary here instead of nullish coalescing?
Uh oh!
There was an error while loading. Please reload this page.
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.
This wasn't bound properly and was causing an error when creating visualize embeddable from saved object:
https://github.com/elastic/kibana/blob/master/src/plugins/visualizations/public/embeddable/visualize_embeddable_factory.tsx#L108