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.22k stars 278 forks source link

Update improper use of `yield` in data stores #6117

Closed nfmohit closed 1 year ago

nfmohit commented 1 year ago

Feature Description

There are a few examples of improper (although non-breaking) generators in our data stores that should be cleaned up.

Example / original description https://github.com/google/site-kit-wp/blob/5a22b076d4bfe47f44f0a8455253a19b056e4fef/assets/js/googlesitekit/modules/datastore/modules.js#L490 This `yield` is unnecessary because it's dispatching directly. If the intention is to await the action, we'd need to use `yield Data.commonActions.await()` around it since the dispatched action will return a promise. In this case, I think we can simply remove it as we don't depend on the response? Ideally, it might be good to use the await action with a `Promise.all` so these can be fetched in parallel. _Originally posted by @aaemnnosttv in https://github.com/google/site-kit-wp/pull/5988#discussion_r1014495179_

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

wpdarren commented 1 year ago

QA Update: ✅

Verified:

aaemnnosttv commented 1 year ago

⚠️ Approval

I found an instance that was missed https://github.com/google/site-kit-wp/blob/cf7d5bc66b934502eb2494cba7621f763d312550/assets/js/googlesitekit/modules/datastore/modules.js#L391-L396

This should be very quick to clean up, so let's address this in a follow-up PR.

aaemnnosttv commented 1 year ago

✅ Approval

Thanks @tofumatt 👍