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! |
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.
4fa6f8e to
6ba4885
Compare
There was a problem hiding this comment.
Summary of what I went through with Claude:
We reviewed the change, reproduced the original warning, and verified the fix end-to-end on a live site. Summary: the change is correct, low-risk, and confirmed to remove the warning.
Code review
- The diff is minimal and right: it keeps
react-google-charts' default package list and adds only thegeochartpackage the chart type actually needs. chartPackagesis a real, public prop (react-google-chartstypes.d.ts) and'geochart'is a validGoogleChartPackagesvalue. The list is passed straight intogoogle.charts.load(version, { packages }).controlsmust stay — not becauseGeoChartneeds it, but becausereact-google-chartsitself does. Its readiness gate requiresgoogle.visualization.ChartWrapperandgoogle.visualization.Dashboard(both from thecontrolspackage); drop it and the gate never passes. That's why it's a library default.corechartis strictly probably not required for a GeoChart, but keeping it is the right call:chartPackagesreplaces the default list rather than extending it, so passing['corechart','controls','geochart']keeps the change a pure superset of the default and avoids any regression. I would not trim it.
Root cause of the version '51' warning
Worth noting since it wasn't obvious: neither this wrapper nor @automattic/charts ever asks for version 51 — the default chartVersion is 'current'. The 51 comes from Google's loader auto-backfilling the missing geochart package at a frozen version after something else on the page already initialized Charts at 'current'. Requesting geochart up front in the initial load avoids that late backfill, which is exactly what this PR does.
Testing
Reproduced and verified on a live wp-admin site (Premium Analytics → Traffic → Top Locations widget):
- Unfixed build (trunk): confirmed the warning fires, and the React component stack pins it precisely to this GeoChart:
GoogleChart → ChartView → GeoChartInternal → GeoChartWithProvider → … → Locations.Attempting to load version '51' of Google Charts, but the previously loaded 'current' will be used instead. - Fixed build (this branch): built
js-packages/charts+premium-analyticswith deps, confirmed the served bundle contains the new code (GEO_CHART_PACKAGES = ["corechart","controls","geochart"]), reloaded, and the warning is gone. Only an unrelatedSelectControl36px deprecation remains. The GeoChart still renders correctly (world map + leaderboard).
Same page, same site, only the package list changed → the warning disappears.
Caveats / suggestions
- This is confirmed for the case where this GeoChart wrapper issues the initial Charts load (the Premium Analytics dashboard). If a different Google Charts consumer initializes the loader first elsewhere (e.g. legacy Stats),
react-google-charts' readiness gate could short-circuit and skip its own load — in that scenario the warning could still appear. Not a blocker for this PR's target surface, just worth being aware of. chartPackagesis effectively undocumented outside the TypeScript types (no mention in the library README or its docs site). A one-line comment onGEO_CHART_PACKAGESexplaining whygeochartis added (and that the list replaces, not extends, the default) would help the next reader.
Net: 👍 from me on correctness and verified behavior.
chartPackages replaces rather than extends react-google-charts' default package list, and the prop is undocumented outside the type definitions. Note both, so the next reader knows why corechart/controls are restated and why geochart must be requested up front. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KFKyku1Lx7uqfJZZCpJD1u
adamwoodnz
left a comment
There was a problem hiding this comment.
LGTM, see previous comment for notes
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.