Charts: Load GeoChart package explicitly#50018
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Code Coverage SummaryCoverage changed in 1 file.
|
nerrad
left a comment
There was a problem hiding this comment.
I'm not confident I understand fully the implications of this change, but on the surface it does look like there were some implicit dependencies on Google Charts being loaded somewhere in the dependency chain (vs explicit dependencies).
Either way, this needs a review by folks more familiar with these dependencies. I'm away for the next week so feel free to take over.
Fixes #
Proposed changes
geochartpackage explicitly when rendering@automattic/chartsGeoChart, while preserving thereact-google-chartsdefaultcorechartandcontrolspackages.@automattic/charts.Why this is being proposed
The Premium Analytics Locations widget renders
GeoChartfrom@automattic/charts. That wrapper delegates toreact-google-charts, whose default package list iscorechartandcontrols.GeoChartitself requires the Google Chartsgeochartpackage. Without requesting that package up front, Google Charts can attempt a later package load after the loader has already initialized with a different version request elsewhere on the page. In the observed page this surfaced as:Attempting to load version '51' of Google Charts, but the previously loaded 'current' will be used instead.This PR keeps the existing
react-google-chartsdefaults and adds only the missinggeochartpackage for theGeoChartcomponent. That makes the component's loader request match the chart type it renders, and avoids relying on Google Charts internals or another page script to backfill the package later.Impact review
@automattic/chartsGeoChartconsumers. Current in-repo consumers are Premium Analytics Locations, Premium Analytics visitors-by-location, and Podcast stats locations. Non-GeoChart consumers of@automattic/chartsare not expected to change because this prop is passed only by the GeoChart wrapper.@automattic/chartsGeoChart. Calypso also consumes@automattic/chartsfor dashboard charts such as monitoring HTTP responses and monitoring request methods, but those components do not use this wrapper. This PR should only matter to Calypso if it later adopts thisGeoChartexport or upgrades an affected bundled consumer.@automattic/chartswrapper. Bundled Dotcom/Stats assets that include@automattic/chartsline-chart code are not expected to change unless they render this package'sGeoChart.react-google-chartsrequest its default version, while ensuring the package list includes the package required by the chart type.Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
pnpm --filter @automattic/charts test -- geo-chart.pnpm --filter @automattic/charts typecheck.pnpm --filter @automattic/charts build.pnpm --filter @automattic/jetpack-premium-analytics typecheck.pnpm --filter @automattic/jetpack-premium-analytics build.