-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Update vega to version 4 #30628
Update vega to version 4 #30628
Conversation
Pinging @elastic/kibana-app |
Jenkins, test this. |
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 was looking through the Vega v4 breaking changes, and it seems the only thing we might have to handle is the new fetch
interface for data loading. This may affect "url": "https://..."
as well as (maybe) type: emsfile
loading. See https://github.com/elastic/kibana/blob/master/src/legacy/core_plugins/vega/public/vega_view/vega_base_view.js#L141 where the v3 data loading is handled.
💔 Build Failed |
💔 Build Failed |
💚 Build Succeeded |
💚 Build Succeeded |
💔 Build Failed |
@@ -116,8 +116,8 @@ describe('VegaParser._parseSchema', () => { | |||
expect(vp.spec).to.eql({ $schema: 'https://vega.github.io/schema/vega/v3.0.json' }); | |||
expect(vp.warnings).to.have.length(1); | |||
}); | |||
it('vega', test('https://vega.github.io/schema/vega/v3.0.json', false, 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.
I think instead modifying those tests, we might want instead to add new tests, since the v3.0 schema should still work. So I think we should just create a new test testing for vega 4.0. And just modify the test that checks for invalid vega schema to v5.0.
Also these test names are super bad, so maybe we could properly rename that test to: should support v3.0 schema
, should support v4.0 schema
, should not support v5.0 schema
. (Why is the test for the newer Vega spec that's not yet supported even called vega old
:D).
@@ -116,8 +116,8 @@ describe('VegaParser._parseSchema', () => { | |||
expect(vp.spec).to.eql({ $schema: 'https://vega.github.io/schema/vega/v3.0.json' }); |
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 wonder if we should switch the default specification to Vega 4 now. But that might break existing Vega visualisations, so I think we can't do it 🤔 Meaning from now on the default spec will always stay Vega 3.0. Kind of stupid, but maybe we should have forbidden specs without a schema in the first place.
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.
Good catch - Suggestion: We remove the default version as a breaking change in 8.0 and leave it at 3.0 for now (in a separate PR)
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 like that suggestion. I'll open an issue, for that plan.
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.
docs/visualize/vega.asciidoc
Outdated
@@ -8,6 +8,10 @@ into Kibana, either standalone, or on top of a map. To see Vega in action, | |||
watch this | |||
https://www.youtube.com/watch?v=lQGCipY3th8[short introduction video]. | |||
|
|||
Currently Vega version 4.4 and VegaLite version 2.6 are supported. | |||
|
|||
NOTE: In Vega it is possible to load data dynamically, e.g. by setting signals as data urls. This is not supported in Kibana as all data is fetched at once prior to passing it to the vega renderer. |
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.
vega renderer
-> Vega renderer
(for consistency)
@nyurik About the the data loading - I tested with http urls and they worked without any problems. |
💚 Build Succeeded |
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.
Code LGTM, Tested with some known broken specs, and seem to work now.
Were you able to load external data via URL without changing kibana.yml file? By default, loading should be prohibited for security reasons. |
@nyurik No, only after changing kibana.yml |
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.
Well done! Turns out fetch
is irrelevant to us, sanitizer api is the same as before. Yeppi! Now time to backport it to 5.5 😄
Just to make things interesting... vega/vega#1561 |
Good job on updating to the new version 👍, next step would be v5, I'm exited now 💯 |
Hi @ctrongduc we are also excited! The vega 5 upgrade is tracked here: #31413 Unfortunately the vega upgrade didn't make it in beta-1, but it should be in the next release. Till then you can check out master and |
Thank you so much for this upgrade! |
@dypsilon This is just about updating vega to version 4, not vega lite - these are two separate libraries with diverging version numbers. Unfortunately the most currently supported version of vega-lite is 2.6 |
@flash1293 I see, thank you for the explanation! |
Summary
Resolves #22644
Updates version of
vega-lib
andvega-lite
to the most recent stable release. Tested against some complex examples from the vega-lite website and didn't encounter any errors so far.Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support- [ ] Unit or functional tests were updated or added to match the most common scenarios- [ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
- [ ] This was checked for breaking API changes and was labeled appropriately