google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.25k stars 289 forks source link

Google Charts collision behaviour causes issues in Storybook/SPA contexts #6708

Open tofumatt opened 1 year ago

tofumatt commented 1 year ago

Bug Description

For those with access to the 10up Slack, this link will provide some context: https://10up.slack.com/archives/CBKKQEBR9/p1678392621467249

Essentially, the fix for the Google Charts instability we saw, largely around other WordPress plugins also loading Google Charts (of a different version), presents its own issues. Mostly that the new Charts check for charts already being loaded and will de-initialise Google Charts if it is loaded on the Site Kit Dashboard context. See this PR: #6461

This means that Storybook, being a Single Page App, will crash Google Charts after navigating around 1-2 times. See:

CleanShot 2023-03-10 at 00 12 14

Removing this line will fix the issue: https://github.com/google/site-kit-wp/blob/2b74402c0c91af37062b446c8adad3d88f2aaaf3/assets/js/components/GoogleChart/index.js#L144

Right now this affects Storybook and no code in production, but this could severely hamper the feasibility of #6650.

Short-term, for testing, we might want to disable the above line when in a Storybook context, but long-term we will likely need a better fix for the underlying issue with Google Charts.

Steps to reproduce

  1. Go to https://google.github.io/site-kit-wp/storybook/develop/?path=/story/modules-searchconsole-widgets-searchfunnelwidget--search-console-zero-state
  2. Click the "Ready" story in SearchFunnelWidget
  3. Click "Ready with Activate Analytics CTA"
  4. Click "Ready with Analytics not active"
  5. See Google Charts in an error state. It continues to be in an error state until the page is refreshed.

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

Implementation Brief

Test Coverage

QA Brief

Changelog entry

aaemnnosttv commented 1 year ago

@tofumatt are you ok to add the AC here or should we have someone else take over?

ivonac4 commented 9 months ago

@tofumatt Are you still planning to work on this soon? If not, could you please unassign yourself so someone else can pick it up? Thank you!