Skip to content

[Maps][Vega] Tidy up mapbox-gl imports#99733

Merged
thomasneirynck merged 8 commits intoelastic:masterfrom
thomasneirynck:maps/tidy_mapboxgl
May 12, 2021
Merged

[Maps][Vega] Tidy up mapbox-gl imports#99733
thomasneirynck merged 8 commits intoelastic:masterfrom
thomasneirynck:maps/tidy_mapboxgl

Conversation

@thomasneirynck
Copy link
Copy Markdown
Contributor

@thomasneirynck thomasneirynck commented May 10, 2021

This PR tidies up some of the mapbox-gl import inconsistencies between Maps and Vega

  • removes redundant css import from Maps
  • uses csp-approved worker loading in vega

This should make the mapbox-gl -> maplibre-gl switch more clean, restricting it to the imports and the yarn.lock change.

Open questions: as suggested here #85566 (comment), we can also isolate mapbox-gl loading in single plugin, further improving the use of the library. this will also reduce total kibana bundle size, since it won't get packaged with two plugins. (although, the dependency is lazy-loaded, it would only affect total kibana size, not necessarily kibana startup time).

@thomasneirynck thomasneirynck added Team:Geo Former Team Label for Geo Team. Now use Team:Presentation release_note:skip Skip the PR/issue when compiling release notes labels May 10, 2021
Copy link
Copy Markdown
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
code review

import mbWorkerUrl from '!!file-loader!mapbox-gl/dist/mapbox-gl-csp-worker';

mapboxgl.workerUrl = mbWorkerUrl;
mapboxgl.setRTLTextPlugin(mbRtlPlugin);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@thomasneirynck can you give me more info about this worker? Why are we adding this? Sorry, I don't have a lot of experience with mapbox :D

Copy link
Copy Markdown
Contributor Author

@thomasneirynck thomasneirynck May 11, 2021

Choose a reason for hiding this comment

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

hi @stratoula, this bring vegamaps in line with how mapbox maps are instantiated in Maps (

// @ts-expect-error
import mbRtlPlugin from '!!file-loader!@mapbox/mapbox-gl-rtl-text/mapbox-gl-rtl-text.min.js';
// @ts-expect-error
import mbWorkerUrl from '!!file-loader!mapbox-gl/dist/mapbox-gl-csp-worker';
mapboxgl.workerUrl = mbWorkerUrl;
mapboxgl.setRTLTextPlugin(mbRtlPlugin);
)

These workers are spawned by mapbox-gl.

This PR changes Vegamaps, so it constructs mapbox with a static-worker script (pointing explicitly to mbWorkerUrl), iso. an inlined string which is the default way mapbox instantiates its workers.

The latter approach was hitting on some possible violations of Kibana's CSP. See #51675.

Apart from that, construction is now the same between Maps and Vega, so it would also give us a pathway to extract mapbox-gl into a separate bundle, reducing footprint.

@thomasneirynck thomasneirynck requested a review from stratoula May 11, 2021 15:19
@thomasneirynck thomasneirynck marked this pull request as ready for review May 11, 2021 15:19
@thomasneirynck thomasneirynck requested a review from a team May 11, 2021 15:19
@elasticmachine
Copy link
Copy Markdown
Contributor

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

@kibanamachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
visTypeVega 241 243 +2

Async chunks

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

id before after diff
visTypeVega 2.6MB 2.6MB -71.1KB
Unknown metric groups

miscellaneous assets size

id before after diff
visTypeVega 0.0B 550.3KB ⚠️ +550.3KB

References to deprecated APIs

id before after diff
canvas 29 25 -4
crossClusterReplication 8 6 -2
fleet 4 2 -2
globalSearch 4 2 -2
indexManagement 12 7 -5
infra 5 3 -2
licensing 18 15 -3
monitoring 109 56 -53
total -73

History

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

Copy link
Copy Markdown
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.

Thank you Thomas, LGTM!

@thomasneirynck thomasneirynck merged commit bc2b846 into elastic:master May 12, 2021
thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request May 12, 2021
VegaMaps now instantiates mapbox-gl the same way as the Maps app.
thomasneirynck added a commit that referenced this pull request May 12, 2021
VegaMaps now instantiates mapbox-gl the same way as the Maps app.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release_note:skip Skip the PR/issue when compiling release notes Team:Geo Former Team Label for Geo Team. Now use Team:Presentation v7.14.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants