Open aaemnnosttv opened 1 month ago
@zutigrm Let's do the opposite, starting with one module. That way the number of files/modules/etc. to test/review is smaller, and the QA can be scoped to testing Analytics behaviour.
If during development there's time to work on more modules they can be added I think.
It's missing from the ACs, but part of this change should include us exporting these new controls from googlesitekit-data
, which is also where we should be importing them from.
I think we can mirror the @wordpress/data
exports and export things as controls
, but if you wanted to alias it to builtInControls
so it can be easily imported without every file needing to do { controls as builtInControls }
that'd be nice, given how many of our files already have a const controls
variable. 👍🏻
Builtin controls can be used in following way: select|resolveSelect|dispatch( storeKey, selectorName, ...args ) , so const propertyID = registry.select( MODULES_ANALYTICS_4 ).getPropertyID() would be replaced with: const propertyID = builtinControls.select( MODULES_ANALYTICS_4, 'getPropertyID' ), etc.
I get what this means but it's a bit hard to read/parse. I think it's fine to write something like:
Replace
registry.select
withbuiltInControls.select
,registry.resolveSelect
withbuiltInControls.resolveSelect
, andregistry.dispatch
withbuiltInControls.dispatch
.
- Builtin controls can be used in following way:
select|resolveSelect|dispatch( storeKey, selectorName, ...args )
, soconst propertyID = registry.select( MODULES_ANALYTICS_4 ).getPropertyID()
would be replaced with:const propertyID = builtinControls.select( MODULES_ANALYTICS_4, 'getPropertyID' )
, etc
I just want to flag that the example here is incorrect. The controls
export is an object of (unbound) action creators in Redux terms. These are then mixed-in with their associated controls (to handle their respective actions) when registering a store.
As mentioned in the description:
These are actions meaning we would
yield
these within our own actions/resolvers instead of using workarounds togetRegistry
andcommonActions.await
just to get access toselect
/registrySelect
ordispatch
andawait
the result.
Calling the function returns the raw action object, so we need to yield
them to dispatch them on the current store.
wpd.controls.select('store/foo', 'getBar', 'argBaz', 'argBuzz')
// returns:
{
type: '@@data/SELECT',
storeKey: 'store/foo',
selectorName: 'getBar',
args: [ 'argBaz', 'argBuzz' ]
}
Thanks @aaemnnosttv indeed, I updated to reference that they should be yielded to return the result.
@tofumatt Thanks for the suggestion, I update IB to mention adding alias for controls to our data package, and applied suggested wording.
We should be explicit in the IB that not all registry.select
calls can be replaced. For instance, any code inside a createRegistryControl
is fine to leave as-is (eg. https://github.com/google/site-kit-wp/blob/dc89f6c19d3d1103e5be3387c2739fe2955324bc/assets/js/googlesitekit/data/create-existing-tag-store.js#L103-L104).
Let's also make a plan to mark getRegistry
as deprecated as part of this issue to discourage future use, as the ACs call for it.
Thanks @tofumatt , that's a good spot. I amended the IB to include those points. I suggested the depreciation error for eslint rule to ensure it is not used. Let me know what you think
I meant a JSDoc @deprecated
notice for now; once all actions are converted we should remove commonActions
instead of marking it as deprecated, but we'll do that once all datastores are migrated.
I adjusted the IB to account for this, and increased the estimate slightly in case any issues are encountered; if they aren't, more work can be done on other stores.
IB ✅
After a discussion with @aaemnnosttv, @eugene-manuilov, and @nfmohit, we're actually going to change the scope of this issue after looking at the resulting PR (#9234), which ends up being a worse API 😅
I'm just updating that PR and will move it to CR soon; I've updated the ACs accordingly.
No QA to do here, moving to approval 👍
Feature Description
With our recent upgrade of
@wordpress/data
, the package now includes a set ofcontrols
which are essentially utility actions that every store will have associatedcontrols
for.These are
select
– Dispatches a control action for triggering a synchronous registry select.resolveSelect
– Dispatches a control action for triggering and resolving a registry select.dispatch
– Dispatches a control action for triggering a registry dispatch.These are actions meaning we would
yield
these within our own actions/resolvers instead of using workarounds togetRegistry
andcommonActions.await
just to get access toselect
/registrySelect
ordispatch
andawait
the result.Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
select
,resolveSelect
, anddispatch
) in ourgooglesitekit-data
packageImplementation Brief
AdSense
module to use the newbuiltInControls
instead of thegetRegistry
common actions. Follow-up issues should be filed to continue to the next module, unless there is time to fit another module in same issue (based on the issue estimate).assets/js/googlesitekit/data/index.js
controls
to the existing imports list from@wordpress/data
, and alias it asbuiltInControls
builtInControls
to the export https://github.com/google/site-kit-wp/blob/2b31bf57bcba33f13dd342c9005ea2879b534723/assets/js/googlesitekit/data/index.js#L74-L100import { builtInControls } from 'googlesitekit-data'
and replace the usage ofawait
,registry
and itsselect
,resolveSelect
anddispatch
in the actions and resolvers, with the builtin controls:yield select( storeKey, selectorName, ...args )
, so replaceregistry.select
withyield builtInControls.select
,registry.resolveSelect
withyield builtInControls.resolveSelect
, andregistry.dispatch
withyield builtInControls.dispatch
builtInControls
object will hold all three controls{select, resolveSelect, dispatch}
commonActions
, we should leave it if it is a passed prop fromcreateRegistryControl
, like here https://github.com/google/site-kit-wp/blob/9306906d89ab39c570f50332bd1bc52522ba35bc/assets/js/modules/analytics-4/datastore/properties.js#L740 and here https://github.com/google/site-kit-wp/blob/dc89f6c19d3d1103e5be3387c2739fe2955324bc/assets/js/googlesitekit/data/create-existing-tag-store.js#L103-L104@deprecated
in JSDoc; no new usage of these actions should be introduced.AdSense
.Test Coverage
QA Brief
googlesitekit-data
.Changelog entry
googlesitekit-data
.