-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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][Vega] Isolate mapbox-gl library into bazel package #99931
[Maps][Vega] Isolate mapbox-gl library into bazel package #99931
Conversation
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Pinging @elastic/kibana-gis (Team:Geo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Windows LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems great Thomas and the removal of 103 KB from vega async bundle makes it even better. Vega changes LGTM! I tested it locally and vega maps work fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
code review
@@ -2678,6 +2678,10 @@ | |||
version "0.0.0" | |||
uid "" | |||
|
|||
"@kbn/mapbox-gl@link:bazel-bin/packages/kbn-mapbox-gl/npm_module": | |||
version "0.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is version 0.0.0 here but 1.0.0 in kbn/mapbox-gl?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. It seems to be pretty consistent, also for other bazel-ackages in the yarn-lock.
cc @mistic do you know why this version is 0.0.0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🎉
Just a CSS file was moved around. There were no actual style changes.
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
Moves mapboxgl library instantiation in separate bazel package.
It is shared by Maps and Vega. Apart from removing duplication, this also avoids duplicate initialization of the workerUrl and RTL-plugin if user both loads Maps and Vega during the same session.
[EDIT]
Discussed offline with @nreese. A follow-on PR will isolate the type-exports in this same package.