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] Isolate maps-setting froms maps_legacy #92918

Merged
merged 33 commits into from
Mar 15, 2021

Conversation

thomasneirynck
Copy link
Contributor

@thomasneirynck thomasneirynck commented Feb 25, 2021

Closes #76532

Partially addresses #89304

  • This extracts the yml map configuration and ems-settings in a separate plugin, called mapsEms
  • In effect, this isolates the maps_legacy dependencies to just region_map and tile_map. maps_legacy now only contains common UX, which is mostly leaflet-based. When removing region_map and tile_map, mapsLegacy can be removed just as well ([Breaking change] Remove Coordinate map and region map visualizations #81703). Removing these plugins, will also resolve the maps-dependencies on kibana_legacy Remove kibana_legacy plugin #76905.
  • introduces some typescript conversions

This PR needs a few follow-on PRs:

@thomasneirynck thomasneirynck added chore [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v7.13.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Mar 1, 2021
@thomasneirynck thomasneirynck marked this pull request as ready for review March 9, 2021 16:57
@thomasneirynck thomasneirynck requested a review from a team March 9, 2021 16:57
@thomasneirynck thomasneirynck requested a review from a team as a code owner March 9, 2021 16:57
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

LGTM, just a few nits
code review, tested in chrome. Tested map.includeElasticMapsService, map.tilemap.url, and map.regionmap and configurations flowed properly to Maps application, vega maps, and tile map and region map

src/plugins/maps_ems/public/plugin.ts Outdated Show resolved Hide resolved
src/plugins/maps_legacy/public/plugin.ts Outdated Show resolved Hide resolved
@thomasneirynck thomasneirynck requested a review from a team as a code owner March 10, 2021 17:01
@@ -107,3 +107,4 @@ pageLoadAssetSize:
osquery: 107090
fileUpload: 25664
banners: 17946
mapsEms: 26072
Copy link
Contributor

@tylersmalley tylersmalley Mar 11, 2021

Choose a reason for hiding this comment

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

Could mapsLegacy be reduced since this was broken out?

Running node scripts/build_kibana_platform_plugins.js --update-limits it looks like mapsLegacy could be set to 87859 which is 5k above the current size.

@stratoula
Copy link
Contributor

@elasticmachine merge upstream

Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

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

Kibana App code changes LGTM! I tested it locally and works as it should 👏

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
mapsEms - 138 +138
mapsLegacy 182 50 -132
total +6

Async chunks

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

id before after diff
mapsEms - 238.1KB ⚠️ +238.1KB
mapsLegacy 520.7KB 283.1KB -237.6KB
regionMap 285.2KB 285.3KB +55.0B
total +585.0B

Page load bundle

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

id before after diff
maps 143.3KB 143.3KB -9.0B
mapsEms - 10.7KB +10.7KB
mapsLegacy 78.9KB 71.2KB -7.8KB
mapsLegacyLicensing 3.4KB 3.4KB -12.0B
regionMap 19.4KB 19.7KB +289.0B
tileMap 18.0KB 17.9KB -77.0B
visTypeVega 57.4KB 57.4KB -27.0B
total +3.1KB
Unknown metric groups

async chunk count

id before after diff
mapsEms - 1 +1

History

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

@thomasneirynck thomasneirynck added the auto-backport Deprecated - use backport:version if exact versions are needed label Mar 15, 2021
@thomasneirynck thomasneirynck merged commit e136385 into elastic:master Mar 15, 2021
@kibanamachine
Copy link
Contributor

💔 Backport failed

❌ 7.x: Commit could not be cherrypicked due to conflicts

To backport manually, check out the target branch and run:
node scripts/backport --pr 92918

thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Mar 15, 2021
Creates a new plugins, maps_ems, with `map.*` configs and shared EMS-settings. `maps_legacy` now only supports the `region_map` and `coordinate_map` plugins.
# Conflicts:
#	packages/kbn-optimizer/limits.yml
thomasneirynck added a commit that referenced this pull request Mar 15, 2021
Creates a new plugins, maps_ems, with `map.*` configs and shared EMS-settings. `maps_legacy` now only supports the `region_map` and `coordinate_map` plugins.
# Conflicts:
#	packages/kbn-optimizer/limits.yml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed chore [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.13.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Decouple maps plugin and vis_type_vega plugin from maps_legacy plugin
6 participants