Embed dashboard by value example & some embeddable clean up

[Dosant]

Summary

1. Added example for using dashboard container by value
   1.1 Refactored embeddable explorer e2e test to use new example, removed not needed kbn_tp_embeddable_explorer plugin. closes Failing test: Plugin Functional Tests.test/plugin_functional/test_suites/embeddable_explorer/dashboard_container·js - embeddable explorer dashboard container "before all" hook for "pie charts" #66863
2. For embeddable explorer examples went away from using getFactoryById() to improve type checks
3. There is new component a replacement for EmbeddableFactoryRenderer with slightly more flexible api: EmbeddableRenderer.
   3.1 We can improve it going forward to support more use case
   3.2 One idea to explore: Dosant/kibana@cf87a82

[Dosant]

input is type safe and infers from embeddable.input

[stacey-gammon]

🎉

[ThomThomson]

Is the doubling up on the input object here just to illustrate the new type safety? Removing it didn't cause any errors on my side, but I didn't fully test this out:

<EmbeddableRenderer
    embeddable={new HelloWorldEmbeddable({ id: 'hello' })}
    input={{ id: 'goodbye' }}
/>

In this case, which input would win out? I assume the one in the input prop would right away overwrite the initialInput of the embeddable.

[Dosant]

1. input is not needed here. will remove 👍
2. input is required only if you pass factory
3. if passing input and embeddable then input inside embeddable takes precedence on initial render, maybe this needs to be changed 🤔

[Dosant]

Removed input in this example

[stacey-gammon]

This is very cool!

Few comments:

• I love how I can edit the input in the dashboard embeddable example, but I think it might need some more error handling. If I make type something that doesn't exist I see no changes.

• The DashboardEmbeddableByValueRenderer is great but I in most usages I expect users will want a reference to the embeddable being rendered because you can listen for changes and sync state to URL. Even in the example, it'd be nice if the input json was two way, and if I put one of the panels in "full screen mode", this was reflected in the input. You could probably add a listener to the react component onInputUpdated.

[stacey-gammon]

🎉

[stacey-gammon]

cc @ThomThomson and @majagrubic - @Dosant is exploring adding an example plugin for embedding dashboards, initially by value, later by reference.

[Dosant]

@stacey-gammon,

I love how I can edit the input in the dashboard embeddable example, but I think it might need some more error handling. If I make type something that doesn't exist I see no changes.

Looks like it is more a problem of downstream embeddable infrastructure pieces. It isn't completely reactive :(
For example, if change not only the type but also the id, then it renders the error.
So assume inner state is not full reacting to input changes.

he DashboardEmbeddableByValueRenderer is great but I in most usages I expect users will want a reference to the embeddable being rendered because you can listen for changes and sync state to URL. Even in the example, it'd be nice if the input json was two way, and if I put one of the panels in "full screen mode", this was reflected in the input. You could probably add a listener to the react component onInputUpdated.

We should expose embeddable! it could be passed in a callback or forwardingRef
I also added onInputUpdated

---

One problem I have is that passing input props internally calls updateInput. But I think it should actually be using something like reloadInput. (we are passing Input, not Partial<Input>).

[stacey-gammon]

Looks like it is more a problem of downstream embeddable infrastructure pieces. It isn't completely reactive :(
For example, if change not only the type but also the id, then it renders the error.
So assume inner state is not full reacting to input changes.

Ohhhh I think that sounds like an issue @ThomThomson ran into. Did you ever fix the issue with containers not having the appropriate update logic?

One problem I have is that passing input props internally calls updateInput. But I think it should actually be using something like reloadInput. (we are passing Input, not Partial).

Oh nooo, yea this will be a problem! It's one of the biggest errors I see users of the system make. It will clobber all the implicit input from a container, making it all explicit (e.g. no more inheriting time range from the dashboard).

I'm not sure of the best way to fix it. I don't love the Partial<Input> type either. I can't recall the details right now but I was discussing with Devon how this creates type issues.

[ThomThomson]

@stacey-gammon, yeah the non-reactive embeddable components issue that @Dosant mentioned is the same as the issue I ran into with the placeholder panel on clone.

I looked into it, and found that the whole Container -> EmbeddableChildPanel -> EmbeddablePanel would need to change to support it, and figured the change would be easier after we are finished with embeddable by reference / value. It's still something I might like to tackle.

[Dosant]

@stacey-gammon,
Current state:

1. Added example for using dashboard container by value
   1.1 Didn't look inside into fixing non-reactive updates issues mentioned above
   1.2 Refactored embeddable explorer e2e test to use new example, removed not needed kbn_tp_embeddable_explorer plugin
2. For embeddable explorer examples went away from using getFactoryById()

Do think this ⬆️ could be checked in?

---

I removed binding output hack and went with explicit export of factory types together with definitions.
Last state of a problem with inferring types is here: #67783 (comment)

---

Regarding updateInput and giving accesses to embeddable object,
This could be way to go:

Dosant@cf87a82

Idea is that EmbeddableRenderer along with other props and handler with forward embeddable instance inside ref.

So consumer can get access to underlying embeddable object

[elasticmachine]

Pinging @elastic/kibana-app-arch (Team:AppArch)

[ThomThomson]

LGTM!

I payed attention specifically to the dashboard input in the dashboard by value example. Everything works great, including all the different behaviors on the dashboard, including cloning.

The questions below are more long-term thoughts / curiosities.

[ThomThomson]

It's good to call out the differences between this and the container.

Maybe I'm missing something, but it looks to me like it would be possible to merge all of this functionality into one component. Which takes over the responsibility of displaying an embeddable given a factory or an embeddable instance, but also subscribes to the parent's input & output (if there is one), and combines those changes with the explicit input on its own.

This change is definitely going in the right direction, but it strikes me as odd that the responsibility of rendering an embeddable that responds to changes is handled in: container, embeddable_renderer, embeddable_panel, and even a little bit in embeddable_child_panel

Just food for thought!

[Dosant]

I agree we can try to dig deeper and this is just a small step into right direction 👍
feel free to try to build on top
As we discussed I didn't look inside EmbeddableRoot yet, I'll maybe also find some time and give something a try soon

[ThomThomson]

Is the doubling up on the input object here just to illustrate the new type safety? Removing it didn't cause any errors on my side, but I didn't fully test this out:

<EmbeddableRenderer
    embeddable={new HelloWorldEmbeddable({ id: 'hello' })}
    input={{ id: 'goodbye' }}
/>

In this case, which input would win out? I assume the one in the input prop would right away overwrite the initialInput of the embeddable.

[Dosant]

@stacey-gammon @streamich, ready for review 🙏

[stacey-gammon]

lgtm!

[streamich]

Code changes LGTM.

[Dosant]

@elasticmachine merge upstream

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 881526a

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
dashboard	334	+1	333
visualize	418	+1	417
apm	1611	+1	1610
dashboardMode	334	+1	333
infra	1611	+1	1610
ml	673	+1	672
transform	740	+1	739
uptime	1410	+1	1409
total	-	+8	-

page load asset size

beta

id	value	diff	baseline
/bundles/plugin/canvas/canvas.plugin.js	3.4MB	+502.0B	3.4MB
/bundles/plugin/dashboard/dashboard.plugin.js	1.1MB	+2.2KB	1.1MB
/bundles/plugin/dashboardEnhanced/dashboardEnhanced.plugin.js	754.2KB	+109.0B	754.1KB
/bundles/plugin/dashboardMode/dashboardMode.plugin.js	1.1MB	+2.2KB	1.1MB
/bundles/plugin/discover/discover.plugin.js	762.6KB	+103.0B	762.5KB
/bundles/plugin/drilldowns/drilldowns.plugin.js	726.6KB	+109.0B	726.5KB
/bundles/plugin/embeddable/embeddable.plugin.js	615.1KB	+507.0B	614.6KB
/bundles/plugin/embeddableEnhanced/embeddableEnhanced.plugin.js	714.3KB	+109.0B	714.2KB
/bundles/plugin/lens/lens.plugin.js	1.9MB	+103.0B	1.9MB
/bundles/plugin/ml/ml.plugin.js	2.0MB	+579.0B	2.0MB
/bundles/plugin/reporting/reporting.plugin.js	914.4KB	+103.0B	914.3KB
/bundles/plugin/uiActionsEnhanced/uiActionsEnhanced.plugin.js	693.0KB	+109.0B	692.9KB
/bundles/plugin/visualizations/visualizations.plugin.js	985.2KB	+103.0B	985.1KB
total	-	+6.8KB	-

History

• 💚 Build #53354 succeeded 54645c8
• 💚 Build #52661 succeeded 49ab828
• 💚 Build #52650 succeeded 6c94e47
• 💔 Build #52458 failed 19c1515
• 💔 Build #52381 failed e3ddfa7

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream