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

[Maps] Add handling for shareable saved objects #111195

Closed
wants to merge 11 commits into from

Conversation

kindsun
Copy link
Contributor

@kindsun kindsun commented Sep 3, 2021

Resolves #101177. Adds handling for shareable saved objects in the Maps app

In Maps app:

image

In Dashboard/Embeddable:

image

Below is an overview of the steps required to migrate current Maps app saved object usage to shareable saved objects using the dev guide

Step 1: Ensure all object links use the root level references field

The Maps app is already doing this. It looks this was introduced in #82486

Step 2: Are there any "deep links" to these objects?

The Maps app uses deep links to load saved maps from the initial landing page. They don't appear to be used anywhere else. Handling has been added for these in this PR.

Step 3: Update your client-side code to correctly handle the three different resolve() outcomes

This is pretty much a continuation of 2 and is covered in this PR

Steps 4, 5 should be handled in a future PR for merge in version 8.0

Testing PR

Follow the instructions for loading sample data outlined here. Review different outcomes in the 3 different spaces created.

@kindsun kindsun added [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v8.0.0 v7.16.0 labels Sep 3, 2021
@kindsun kindsun marked this pull request as ready for review September 16, 2021 15:29
@kindsun kindsun requested a review from a team as a code owner September 16, 2021 15:29
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis (Team:Geo)

@kindsun kindsun added the release_note:skip Skip the PR/issue when compiling release notes label Sep 16, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
maps 846 847 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
maps 3.2MB 3.2MB +1.9KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
maps 80.8KB 80.9KB +123.0B

History

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


interface MapRefreshConfig {
isPaused: boolean;
interval: number;
}

export interface Props {
spacesApi?: SpacesPluginStart;
http: HttpStart;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

http does not need to be passed to MapApp, its never used. This dependency chain starts all the way in RenderApp. No need to pass http to RenderApp in public/plugin

const UsageTracker =
plugins.usageCollection?.components.ApplicationUsageTrackingProvider ?? React.Fragment;
const { renderApp } = await lazyLoadMapModules();
return renderApp(params, UsageTracker);
return renderApp(params, UsageTracker, http, spacesApi);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure passing spaces into renderApp this way is needed. Spaces is a start dependency and can just be grabbed from kibana_services when needed to align with how all start dependencies are obtained.


interface MapRefreshConfig {
isPaused: boolean;
interval: number;
}

export interface Props {
spacesApi?: SpacesPluginStart;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Get spacesStartApi from kibana_services instead of passing in to be consistent with how other dependencies are provided

@@ -152,6 +153,23 @@ export class MapEmbeddable
this.onFatalError(e);
return;
}
try {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of handing saved object check seperatly in MapEmbeddable and MapApp, how about moving logic into a new SavedMap called something like "hasSavedObjectIdConflict"? That way all checking logic can just be part of SavedMap and consolidated into a single location.

@nreese
Copy link
Contributor

nreese commented Sep 16, 2021

Looks like Lens has already migrated to using SavedObjectClient.resolve. We should try to stay aligned with how Lens has migrated to SavedObjectClient.resolve and instead of making a separate call to SavedObjectClient.resolve, it looks like unwrapAttributes returns sharingSavedObjectProps. We should just be able to use those values instead of needing resolve_saved_object method.

Then in the embeddable, just get sharingSavedObjectProps from attributes like this
https://github.com/elastic/kibana/blob/master/x-pack/plugins/lens/public/embeddable/embeddable.tsx#L283

@nreese
Copy link
Contributor

nreese commented Sep 20, 2021

Replacing with #112606

@nreese nreese closed this Sep 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation release_note:skip Skip the PR/issue when compiling release notes v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[8.0 only] Make "map" saved object share-capable
4 participants