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] Communicate Data View Ids #137493

Open
ThomThomson opened this issue Jul 28, 2022 · 5 comments
Open

[Embeddables] Communicate Data View Ids #137493

ThomThomson opened this issue Jul 28, 2022 · 5 comments
Labels
Feature:Embeddables Relating to the Embeddable system impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:large Large Level of Effort Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas

Comments

@ThomThomson
Copy link
Contributor

ThomThomson commented Jul 28, 2022

How it works now
Dashboard needs to know which data views its child embeddables use in order to provide appropriate options when creating a filter or a control. Currently, most embeddables communicate their data views via their output. Dashboard reads the output, casts it and tries to grab the data view object from it.

Problems with this approach:

  • Since Dashboard doesn't know what each embeddable is outputting it needs to subscribe to every output change for every one of its children. Even if the output change is not relevant, dashboard will recalculate its list of data views.
  • The entire Data View instance is communicated through the embeddable's output. This is non-serializable, and the objects can get quite large.

What should we do instead?:
Instead, it would be a better pattern to create a new interface something like ICommunicatesDataViews, which comes with a method to access a Subject<string> called $onDataViewChange that the embeddable can publish to when the data view changes.

Notice this subject is of type string because it should only publish the data view Id, because the data views in use should already be cached in the data views service. We can access them this way rather than passing the full class instance around.

Another good change would be to make the top nav bar / the unified search bar take an array of dataViewIds, rather than an array of pre-fetched Data View objects.

More advanced fix
A more complex fix would be to add a system that lets embeddable parents easily subscribe to changes in certain states of their children.

CC @dokmic @flash1293, any feedback or other options is welcome!

@ThomThomson ThomThomson added Feature:Dashboard Dashboard related features Feature:Embedding Embedding content via iFrame Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas loe:large Large Level of Effort Team:AppServicesSv impact:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. labels Jul 28, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@elasticmachine
Copy link
Contributor

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

@exalate-issue-sync exalate-issue-sync bot added loe:small Small Level of Effort and removed loe:large Large Level of Effort labels Aug 17, 2022
@vadimkibana
Copy link
Contributor

In general we have this architecture, where embeddable reports their state through the embeddable "output". (I'm not saying I'm a big fan of it.) But we have it, and it seems that embeddable "output" can be used here to subscribe to all embeddable data view changes; the whole data view does not need to be extracted - the output observable can be mapped to just data view ID, and further observable distinctUntilChanged() can be used to not fire observable events if ID does not change, effectively achieving the same.

Something like:

const dataViewId$ = embeddable.output$.pipe(
  map(dataView => dataView.id),
  distinctUntilChanged(),
);

@exalate-issue-sync exalate-issue-sync bot removed the loe:small Small Level of Effort label Sep 26, 2022
@ThomThomson
Copy link
Contributor Author

@vadimkibana this is the current implementation.

@ThomThomson ThomThomson added the loe:large Large Level of Effort label Apr 10, 2023
@ThomThomson ThomThomson added Feature:Embeddables Relating to the Embeddable system and removed Feature:Embedding Embedding content via iFrame labels Jun 14, 2023
@ThomThomson ThomThomson changed the title [Dashboard][Embedding] Communicate Data View Ids [Embeddables] Communicate Data View Ids Oct 5, 2023
@ThomThomson ThomThomson removed the Feature:Dashboard Dashboard related features label Oct 5, 2023
@ThomThomson
Copy link
Contributor Author

This will be resolved by the Embeddables Rebuild project.

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:medium Addressing this issue will have a medium level of impact on the quality/strength of our product. loe:large Large Level of Effort Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas
Projects
None yet
Development

No branches or pull requests

4 participants