Update @wordpress/data to the latest version

[felixarntz]

Feature Description

While updating dependencies via #1356, it became apparent that there were some issues with @wordpress/data, which has prevented us from updating it to its latest version as well as preventing us from updating to the latest versions of react and react-dom.

Currently we're using react and react-dom 16.12.0 and @wordpress/data 4.12.0.

When updating @wordpress/data to a version newer than 4.12.0, we run into an error whenever useSelect is used.

When updating only react and react-dom to a version newer than 16.12.0, the following error is thrown when opening a Site Kit module's settings view:

Warning: Cannot update a component (`%s`) while rendering a different component (`%s`). To locate the bad setState() call inside `%s`, follow the stack trace as described in https://fb.me/setstate-in-render%s PermissionsModal AdBlockerWarning AdBlockerWarning 
        in AdBlockerWarning
        in Unknown
        in InnerComponent (created by WithFilters(ModuleSettingsWarning))
        in WithFilters(ModuleSettingsWarning)
        in div
        in SetupModule
        in SettingsModule
        in div
        in div
        in div
        in div
        in Layout
        in div
        in SettingsModules
        in div
        in div
        in div
        in SettingsApp
        in RestoreSnapshots
        in ErrorHandler
        in Root

Looking into the latter, it seems this problem is also somehow caused by the useSelect hook implemented by @wordpress/data. Let's dive deeper into what the problem is here. (Note that WordPress packages currently rely on an older React version 16.9.0.)

• useSelect which we're using
• latest useSelect

Potentially we will end up blocked by @wordpress/data fixing the underlying issue - let's file an upstream issue / pull-request about the problem if we can figure out what it is.

---

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

Acceptance criteria

• @wordpress/data should be updated to its latest version (currently 4.27.3) 4.23.0 (as a first step towards later versions).
  • react and react-dom may be updated but only as necessary
• No JavaScript errors and no new warnings should be triggered when visiting any Site Kit UI.

Implementation Brief

Please note: This IB has been scoped to updating to @wordpress/data version 4.23.0, as that is the version that introduces the change that breaks the majority of our tests (see this comment below). Subsequent issues should be raised, one for updating to version 4.27.3 (the latest version that supports React 16), and another to then investigate updating to a more recent version which supports React 17 (currently, the latest @wordpress/data version is 7.6.0).

• Update to @wordpress/data version 4.23.0.
• Ensure the plugin continues to work as usual.
• Create a followup issue to upgrade to version 4.27.3.
• Create a subsequent followup issue to investigate updating to the latest version.

Test Coverage

• Fix all failing tests.
• The most effective way of fixing the tests takes its initial cue from the previous PR for the issue which is now a bit out of date. That is, to enable Jest's real timers for all tests in a global beforeAll. This reduces the initial failing test count from 813 to 213 at the time of writing. However when it comes to fixing the remaining tests we can also use the approach that the WordPress devs use (again see the comment below), switching to fake timers and calling jest.runAllTimers() as needed. Additionally the techniques seen in the previous PR are still useful.
• A new PR to kick this off has been created which fixes a number of these remaining tests (55, to be precise) in a few different ways. It's working in the browser, although there are a couple of failing E2E tests to take a look at. The VRT tests are currently failing but that's a general issue, although for safety's sake we should wait for Fix the VRT tests that are failing in CI. #6185 to be merged so we can make a proper comparison. Storybook itself appears to be OK.
• The above mentioned PR should be picked up and finished off.

Note: The list of remaining failing test suites is as follows:

This is correct at the time of writing, to recreate the list run this command:

> npm run test:js 2>&1 | grep FAIL | sort | uniq

assets/js/googlesitekit/datastore/user/authentication.test.js (11.239s)
assets/js/googlesitekit/datastore/user/dismissed-items.test.js
assets/js/googlesitekit/datastore/user/feature-tours.test.js (11.267s)
assets/js/googlesitekit/datastore/user/permissions.test.js
assets/js/googlesitekit/datastore/user/surveys.test.js
assets/js/googlesitekit/datastore/user/user-info.test.js (11.282s)
assets/js/googlesitekit/modules/datastore/modules.test.js (12.046s)
assets/js/googlesitekit/modules/datastore/sharing-settings.test.js
assets/js/hooks/useChecks.test.js
assets/js/hooks/useCompleteModuleActivationCallback.test.js
assets/js/hooks/useDebouncedState.test.js
assets/js/modules/adsense/components/module/ModuleOverviewWidget/Footer.test.js
assets/js/modules/adsense/datastore/accounts.test.js
assets/js/modules/adsense/datastore/adblocker.test.js
assets/js/modules/adsense/datastore/alerts.test.js
assets/js/modules/adsense/datastore/clients.test.js
assets/js/modules/adsense/datastore/report.test.js
assets/js/modules/adsense/datastore/sites.test.js
assets/js/modules/analytics-4/datastore/webdatastreams.test.js (11.164s)
assets/js/modules/analytics/components/common/AccountSelect.test.js
assets/js/modules/analytics/components/common/PropertySelectIncludingGA4.test.js
assets/js/modules/analytics/components/module/ModulePopularPagesWidget/Footer.test.js
assets/js/modules/analytics/components/settings/SettingsEdit.test.js
assets/js/modules/analytics/datastore/accounts.test.js (11.474s)
assets/js/modules/analytics/datastore/adsense.test.js
assets/js/modules/analytics/datastore/goals.test.js
assets/js/modules/analytics/datastore/profiles.test.js
assets/js/modules/analytics/datastore/properties.test.js
assets/js/modules/analytics/datastore/report.test.js (11.053s)
assets/js/modules/analytics/datastore/setup-flow.test.js
assets/js/modules/idea-hub/components/dashboard/DashboardIdeasWidget/index.test.js (16.465s)
assets/js/modules/pagespeed-insights/datastore/report.test.js
assets/js/modules/search-console/components/dashboard/SearchFunnelWidget/Footer.test.js
assets/js/modules/search-console/components/dashboard/SearchFunnelWidget/index.test.js
assets/js/modules/search-console/components/dashboard/SearchFunnelWidget/Overview.test.js
assets/js/modules/search-console/datastore/report.test.js (10.995s)
assets/js/modules/tagmanager/components/settings/SettingsEdit.test.js (14.372s)
assets/js/modules/tagmanager/components/setup/SetupMain.test.js (14.197s)
assets/js/modules/tagmanager/datastore/accounts.test.js (11.965s)
assets/js/modules/tagmanager/datastore/containers.test.js
assets/js/modules/tagmanager/datastore/versions.test.js
assets/js/modules/thank-with-google/datastore/publications.test.js

QA Brief

• This needs a really thorough regression test of the entirety of Site Kit - every module and feature.
• Focus on the behavioural aspects, this is an upgrade of the JS data library that underpins the UI, so we need to be sure the plugin still behaves as expected. It's unrelated to the styling, though, so there's no need to dig into that too deeply, as long as it all looks as expected it can be assumed the styles are unaffected.
• This change also includes a minor React version bump, 16.12.0 -> 16.14.0. All Site Kit UI should be checked for React errors but also non-fatal React errors/warnings in the console.

Changelog entry

• Upgrade @wordpress/data to 4.23.0, react and react-dom to 16.14.0.

[felixarntz]

@tofumatt This is still blocked by #1761 though right?

[tofumatt]

These are the warnings I see in the console, but they aren't errors and don't impact tests/usability of the plugin:

That said, we could mark this as blocked by that issue so these warnings don't appear in the plugin. They do only appear in development mode, so the warnings won't appear in the production version of the plugin (here's the same site running the version created by npm run build:

I wouldn't consider it a blocker because those notices only appear in development.

[felixarntz]

IB ✅

[tofumatt]

Further testing of this shows useInstanceId is causing hooks errors even without @wordpress/data or React upgraded.

[tofumatt]

Moving this back to the execution backlog with a much higher estimate, as this issue has totally eluded me.

Upgrading @wordpress/data to 4.13 from 4.12 breaks nearly every test, but strangely it seems to only be in the tests—largely Jest tests. The diff is here: https://diff.intrinsic.com/@wordpress/data/4.12.0/4.13.0#file-src__components__use-dispatch__use-dispatch-with-map.js and aside from a move to useMemoOne there isn't a lot there that's changed.

We should plan to upgrade these critical dependencies, but it will take a lot more time.

---

That said, there's a further discussion to be had about our usage of @wordpress/data. It's a system largely only used by Gutenberg and seemingly not as a third-party dependency by many other projects directly. The changelogs are not reliable/up-to-date, and we are possibly one of the larger non-WordPress Core projects using it. When we encounter issues like this, it's tough to find others with similar problems. Something to consider, but that's about all for now 😄

[aaemnnosttv]

Sounds good, thanks for clarifying @techanvil !

IB ✅ (part deux 🚀)

[aaemnnosttv]

@techanvil would you please open the next you've issues identified here?

[techanvil]

Thanks for calling that out @aaemnnosttv. I've created followups #6356 and #6357.

[wpdarren]

QA Update: ✅

Verified:

• Completed a smoke test of every module and feature within Site Kit.
• Focused on the behavioural aspects, in particular the UI.
• Site Kit UI was checked for React errors but also non-fatal React errors/warnings.

Spent quite a lot of time going through the plugin and checking messages in the console. Also enabled services like Dashboard Sharing, GA4 Activation Banner to ensure that these areas do not have any errors or UI issues. The QA team is continuing to be cautious while completing testing on other tickets to ensure we are thorough.

[aaemnnosttv]

I noticed a console error (React warning) when changing the date range on the dashboard, but this appears to be coming from react-google-charts. It does not seem to happen in the latest release but this warning will only appear in development builds and is probably related to the update to React here. It's not something to block the issue or release over so we can look into addressing this going forward, as it is also a known issue on the chart package repo.

Screenshot


Happy to finally close this one out after more than 2 years in the making 🎉

[wpdarren]

It's not something to block the issue or release over so we can look into addressing this going forward

@aaemnnosttv is it worth creating a ticket for this?