[Example] Embeddable by Reference and Value #68719
Conversation
ThomThomson
added
Feature:Embedding
ThomThomson
added
Team:Visualizations
|
Pinging @elastic/kibana-app-arch (Team:AppArch) |
examples/embeddable_examples/public/book/book_embeddable_factory.tsx
Outdated
Show resolved
Hide resolved
| > { | ||
| constructor(private type: string, private savedObjectsClient: SavedObjectsClientContract) {} | ||
|
|
||
| public async unwrapAttributes(input: RefType | ValType): Promise<SavedObjectAttributes> { |
There was a problem hiding this comment.
We assume that saved object's attributes are matching embeddable input, correct?
But this isn't the case, for example, for dashboardContainer, right?
I wonder if we should keep this in mind and allow to provide custom mappers? Or are we consider dashboardContainer a legacy example?
Or is my example not related at all?
There was a problem hiding this comment.
We aren't assuming that the saved object's attributes match the attributes key in embeddable input, because attribute_service actually requires it (which comes with its own problems).
When creating an instance of attribute_service, you need to provide a type argument for the saved object attributes AttributeType but also, the value type argument must extend { attributes: AttributeType }. This means that any embeddable that wants to make use of this service must follow an exact convention.
I certainly wouldn't say that this convention would mark any other types of embeddable as legacy. The idea is that it's more of an optional service (helper maybe?) to make it easier to adopt the 'by value OR by reference' paradigm.
The custom mappers you suggest might make this a little more flexible. For instance, a valueAttributesMapper function param which takes the 'by value' input and returns the attributes, but it seems like that would require method level type parameters instead of the current class level ones.
Do you have any thoughts on if it would be best to generalize this service, or leave it as an optional helper with very stringent requirements?
There was a problem hiding this comment.
@ppisljar, can you look into this and provide your thoughts?
There was a problem hiding this comment.
i am wondering if there is any existing embeddable this can be used for ? or do we know for sure of future embddable that would need this ? it feels we don't have the scenario where embeddable input would equal saved object attributes, also loading of saved objects in most cases happen in special way (is not general).
if that is the case then i would vote for not having this service at all and leave it up to the embeddable implementation to do this (if that is anyway what they would need to do, at least for current embeddables)
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
merge conflict between base and head |
| return true; | ||
| } | ||
|
|
||
| public createFromSavedObject = async ( |
There was a problem hiding this comment.
I didn't notice that it was optional! I've removed this method.
|
|
||
| public async unwrapAttributes(input: RefType | ValType): Promise<SavedObjectAttributes> { | ||
| if (isSavedObjectEmbeddableInput(input)) { | ||
| const savedObject: SimpleSavedObject<SavedObjectAttributes> = await this.savedObjectsClient.get< |
There was a problem hiding this comment.
we are using saved object client to get a saved object .... our general approach in kibana is to actually use a higher level abstraction (savedVisualizationLoader, savedSearchLoader, indexPatternsService) which will usually perform quite some magic on top of getting the saved object.
i am wondering do we/will we have any embeddable where saved object attributes are gonna exactly equal embeddable input ?
There was a problem hiding this comment.
As discussed in our sync -- just documenting here -- The attribute service is meant to be used as a higher level abstraction in multiple places including, lens & visualize to provide a standardized pattern for using Embeddables by reference or value.
| > { | ||
| constructor(private type: string, private savedObjectsClient: SavedObjectsClientContract) {} | ||
|
|
||
| public async unwrapAttributes(input: RefType | ValType): Promise<SavedObjectAttributes> { |
There was a problem hiding this comment.
i am wondering if there is any existing embeddable this can be used for ? or do we know for sure of future embddable that would need this ? it feels we don't have the scenario where embeddable input would equal saved object attributes, also loading of saved objects in most cases happen in special way (is not general).
if that is the case then i would vote for not having this service at all and leave it up to the embeddable implementation to do this (if that is anyway what they would need to do, at least for current embeddables)
💚 Build SucceededBuild metrics@kbn/optimizer bundle module count
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
I tested this example and works and looks great.
As discussed in our sync with @ppisljar two days ago - this is the approach we'd like to take with Lens and Visualize embeddables. We'll need to add some tests for AttributeService before we start using it in production embeddables, but not a blocker for now.
Added an attribute service to embeddable start contract which provides a higher level abstraction for embeddables that can be by reference OR by value. Added an example that uses this service.
Summary
This PR is a combination of #62698 and #64131.
In short, it provides an example book embeddable which does the following:
Checklist
For maintainers