-
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
Upgrade elastic charts v43.1.1 #121593
Upgrade elastic charts v43.1.1 #121593
Conversation
Pinging @elastic/datavis (Feature:ElasticCharts) |
Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors) |
@stratoula Ok sorry for the delay. I just updated the branch with the two bug fixes that you mentioned. Ready for your review when you have time. Thanks! |
@elasticmachine merge upstream |
@elastic/kibana-operations could you review 81d7707? We removed the need for the ResizeObserver pollyfill in charts and noticed all the jest tests failed. Are you ok adding this pollyfill to the jest preset setup? Example error ⬇️ Link
|
@elasticmachine merge upstream |
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.
@nickofthyme thank you so much for fixing everything! I can't find any other regression! Code LGTM!
@@ -18,3 +18,5 @@ if (!global.URL.hasOwnProperty('createObjectURL')) { | |||
// Will be replaced with a better solution in EUI | |||
// https://github.com/elastic/eui/issues/3713 | |||
global._isJest = true; | |||
|
|||
global.ResizeObserver = require('resize-observer-polyfill'); |
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.
It looks like we typically use ResizeObserver
via import { ResizeObserver } from 'resize-observer-polyfill'
and not via the global. We don't currently do something like this in the browser, and if this polyfill is necessary for @elastic/charts
we should probably include the polyfill in packages/kbn-ui-shared-deps-src/src/polyfills.js
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.
Hey @spalger actually the ResizeObserver is supported by all major browsers we support and we don't need anymore the polyfill.
That's why I've removed the polyfill from the @elastic/charts
package.
The problem is that the ResizeObserver
object is not available in the window
object of JSDOM when running the Jest test. Looks like it is not implemented in JSDOM@26.
I don't think we need to include it into packages/kbn-ui-shared-deps-src/src/polyfills.js
and I don't think I need to revert our changes in charts to bring back that polyfill. The polyfill should just live in the test environment because is not supported, but we should not include it in our distribution. What do you think?
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 I've found the issue: in TS the ResizeObserver
object is available as a global and it got compiled correctly.
Instead, in JSDOM it is not global but is only a window
object.
I've changed the chart compiled code to use window.ResizeObserver
instead and the tests are passing. I will push an update into charts soon to fix that
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.
Nice, thanks!
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.
Actually, this doesn't solve the problem, I can't figure it ATM so I'm reverting back our commit in the elastic-charts and to include back again the polyfill.
This is anyway an issue that needs to be solved: JSDOM doesn't mock/support the ResizeObserver API, Kibana is using 3 years old with no commits resize-observer-polyfill library.
1296e36
to
417c401
Compare
💚 Build SucceededMetrics [docs]Async chunks
Public APIs missing exports
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.
RUM charts 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.
Looked at the code changes and did a quick scan of APM app charts, LGTM!
We bring back the polyfill removing the changes made to the jest polyfill global env #121593 (comment)
Summary
This PR updates
@elastic/charts
tov43.0.0
which included some breaking changes listed below per version.v41.0.0
Removal of
config
fromPartition
,Heatmap
,Goal
andWordcloud
specs. The config props were moved to either the respective spec or theme section. See details breaking changes fromv41.0.0
.v42.0.0
v43.0.0
Followup needed to deprecate
EuiChartTheme.partition
in elastic/eui#5492. Workaround used for now.Checklist