Implementing ReferenceOrValueEmbeddable for visualize embeddable #76088

majagrubic · 2020-08-27T14:22:19Z

Summary

This PR implements ReferenceOrValueEmbeddable for visualize embeddable, so that we can easily switch between by-value and by-reference embeddable. It uses AttributeService where appropriate.

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

src/plugins/visualizations/public/embeddable/visualize_embeddable.ts

elasticmachine · 2020-09-10T14:41:57Z

Pinging @elastic/kibana-app (Team:KibanaApp)

ThomThomson

Tested locally in chrome, everything works as it should.

I did notice a few small inconsistencies with custom panel titles and time ranges, during the add / unlink actions but they are fixed in the next version of lens by value so it would be best not to address them here.

I have a few small questions/comments about the code but otherwise everything LGTM

ThomThomson · 2020-09-10T21:29:55Z

src/plugins/visualizations/public/embeddable/visualize_embeddable.ts

+    return this.attributeService.inputIsRefType(input as VisualizeByReferenceInput);
+  };
+
+  getInputAsValueType = async (): Promise<VisualizeByValueInput> => {


Why not use the attribute service here as well? The implementation is quite simple, but it could be more consistent that way.

I tried, but it's honestly too much hassle. I'd need a customUnwrapMethod and the code becomes so much more complicated. I don't think it's worth the effort.

src/plugins/visualizations/public/embeddable/visualize_embeddable_factory.tsx

ThomThomson · 2020-09-10T21:34:37Z

src/plugins/embeddable/public/lib/embeddables/default_embeddable_factory_provider.ts

-      ((savedObjectId: string, input: Partial<I>, parent?: IContainer) => {
-        throw new Error(`Creation from saved object not supported by type ${def.type}`);
-      }),
+    createFromSavedObject: def.createFromSavedObject


I'm not quite sure why this change is required, why the bind, and why a ternary here instead of nullish coalescing?

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

ThomThomson · 2020-09-10T21:35:12Z

src/plugins/visualizations/public/embeddable/visualize_embeddable.ts

+  savedVis?: VisSavedObject;
+};
+export type VisualizeByValueInput = { attributes: VisualizeSavedObjectAttributes } & VisualizeInput;
+export type VisualizeByReferenceInput = SavedObjectEmbeddableInput & VisualizeInput;


Nice looking input shapes!

ThomThomson · 2020-09-10T21:45:58Z

src/plugins/dashboard/public/attribute_service/attribute_service.tsx

+        if ('id' in savedItem) {
+          return { ...originalInput, savedObjectId: savedItem.id } as RefType;
+        }
+        return { ...originalInput } as RefType;


I'm wondering about this case for when there isn't an id in the returned saved item - is this part of the error case that you've added to the API? If so, shouldn't we return the error?

The id check here is to make typescript happy. But good point that we need error handling for the scenario where saving a saved object fails

Okay yes, makes sense. You need the check because you changed the signature of the custom save method to send back an error case.

majagrubic · 2020-09-14T09:13:43Z

@elasticmachine merge upstream

ppisljar

code LGTM

kibanamachine · 2020-09-14T10:47:29Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: cee9b81

Build metrics

page load bundle size

id	value	diff	baseline
dashboard	711.6KB	+140.0B	711.5KB
embeddable	430.6KB	-71.0B	430.7KB
visualizations	412.5KB	+5.6KB	406.9KB
total		+5.7KB

History

💚 Build #73731 succeeded e645b98
💔 Build #73687 failed 3e7668b
💔 Build #73677 failed 1189f44
💚 Build #73370 succeeded db9adc0
💔 Build #73346 failed c04d0f7

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

ThomThomson

Tested locally on chrome - everything still works great, and all my questions have been answered.

LGTM!

lukeelmers · 2020-09-14T20:19:13Z

src/plugins/visualizations/kibana.json

@@ -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"],


Are we certain the only way to address this is to introduce a required dependency between visualizations and dashboard?

This doesn't feel right to me, but I'm also not familiar enough with the AttributeService to understand whether alternatives are possible.

I just wanted to raise the question before we move forward with merging this PR.

Hey Luke! This is a really good point.

I made the decision to move attributeService out of the embeddable plugin and temporarily into the dashboard plugin in #74302. It was moved because it makes use of the savedObjectsClient, and @streamich and I had a discussion about how best to avoid coupling savedObjects and embeddables.

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?

hmm, right i didn't notice this ...
saved objects is its own plugin, why did we prefer having attribute service in dashboard plugin instead of embeddable ? it seems embeddable specific ? if we don't want to add saved objects dependency to embeddable maybe this should be its own plugin ? shouldn't be a problem now that we can group plugins under domain folders

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.
I am going to merge this PR now as much of the logic will remain unaffected regardless of where the attribute service lives. If concerns exists, I will revisit this in a follow-up PR.

) (#77457) * Adding dashboard as dependency to visualize * Making visualize embeddable as ReferenceOrValueEmbeddable * Adding attribute service to visualize embeddable * Binding createFromSavedObject method * Add/remove visualize embeddable from library * Removing debugger statement * Using custom save method on attribute service * Reverting to not using attribute service * Resetting flag value * Fix i18n title and update title * Using custom save method on attribute service * Fixing eslint * Using custom save method in visualize embeddable factory * Making getInputAsValueType return Promise as it should * Better error handling when saving a visualization fails Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

Maja Grubic added 7 commits August 23, 2020 20:13

Adding dashboard as dependency to visualize

c014e35

Making visualize embeddable as ReferenceOrValueEmbeddable

dbafccb

Adding attribute service to visualize embeddable

7f2e4ad

Merge branch 'master' into vis-by-value-or-reference

3209dc2

Binding createFromSavedObject method

9788589

Add/remove visualize embeddable from library

2b6f075

Removing debugger statement

e9dff6f

majagrubic commented Aug 27, 2020

View reviewed changes

src/plugins/visualizations/public/embeddable/visualize_embeddable.ts Outdated Show resolved Hide resolved

Maja Grubic added 9 commits September 7, 2020 09:46

Merge master

3c02a0e

Using custom save method on attribute service

f04c20f

Reverting to not using attribute service

d9e8bfb

Resetting flag value

11e878a

Fix i18n title and update title

020c104

Using custom save method on attribute service

c04d0f7

Fixing eslint

db9adc0

Using custom save method in visualize embeddable factory

1189f44

Making getInputAsValueType return Promise as it should

3e7668b

majagrubic marked this pull request as ready for review September 10, 2020 14:40

majagrubic requested a review from a team September 10, 2020 14:40

majagrubic requested a review from a team as a code owner September 10, 2020 14:40

majagrubic added Feature:Dashboard Dashboard related features release_note:roadmap Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.1.0 labels Sep 10, 2020

Merge branch 'master' into vis-by-value-or-reference

e645b98

botelastic bot added the Feature:Embedding Embedding content via iFrame label Sep 10, 2020

majagrubic changed the title ~~[DRAFT] Implementing ReferenceOrValueEmbeddable for visualize embeddable~~ Implementing ReferenceOrValueEmbeddable for visualize embeddable Sep 10, 2020

majagrubic requested review from ThomThomson and ppisljar September 10, 2020 15:20

ThomThomson reviewed Sep 10, 2020

View reviewed changes

Better error handling when saving a visualization fails

5843645

Merge branch 'master' into vis-by-value-or-reference

cee9b81

ppisljar approved these changes Sep 14, 2020

View reviewed changes

ThomThomson approved these changes Sep 14, 2020

View reviewed changes

lukeelmers reviewed Sep 14, 2020

View reviewed changes

majagrubic merged commit e6e7187 into elastic:master Sep 15, 2020

majagrubic deleted the vis-by-value-or-reference branch September 15, 2020 09:18

majagrubic mentioned this pull request Sep 15, 2020

[7.x] Implementing ReferenceOrValueEmbeddable for visualize embeddable (#76088) #77457

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementing ReferenceOrValueEmbeddable for visualize embeddable #76088

Implementing ReferenceOrValueEmbeddable for visualize embeddable #76088

majagrubic commented Aug 27, 2020 •

edited

Loading

elasticmachine commented Sep 10, 2020

ThomThomson left a comment

ThomThomson Sep 10, 2020

majagrubic Sep 14, 2020

ThomThomson Sep 10, 2020

majagrubic Sep 14, 2020 •

edited

Loading

ThomThomson Sep 10, 2020

ThomThomson Sep 10, 2020

majagrubic Sep 14, 2020 •

edited

Loading

ThomThomson Sep 14, 2020

majagrubic commented Sep 14, 2020

ppisljar left a comment

kibanamachine commented Sep 14, 2020

ThomThomson left a comment

lukeelmers Sep 14, 2020

ThomThomson Sep 14, 2020

ppisljar Sep 15, 2020

majagrubic Sep 15, 2020

Implementing ReferenceOrValueEmbeddable for visualize embeddable #76088

Implementing ReferenceOrValueEmbeddable for visualize embeddable #76088

Conversation

majagrubic commented Aug 27, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Sep 10, 2020

ThomThomson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majagrubic Sep 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majagrubic Sep 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majagrubic commented Sep 14, 2020

ppisljar left a comment

Choose a reason for hiding this comment

kibanamachine commented Sep 14, 2020

💚 Build Succeeded

Build metrics

page load bundle size

History

ThomThomson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majagrubic commented Aug 27, 2020 •

edited

Loading

majagrubic Sep 14, 2020 •

edited

Loading

majagrubic Sep 14, 2020 •

edited

Loading