Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor waitFor* actions with resolveSelect. #9978

Merged
merged 21 commits into from
Jan 17, 2025
Merged

Refactor waitFor* actions with resolveSelect. #9978

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
05c9fce
Remove the `waitForContainers()` action, add a resolver for `getWebCo…
techanvil Jan 3, 2025
52e24bb
Remove the `waitForHTMLForURL()` action.
techanvil Jan 3, 2025
2f1270a
Remove the `waitForPropertySummaries()` action, add a resolver for `g…
techanvil Jan 3, 2025
dc7719c
Remove the `waitForWebDataStreams()` action, add a resolver for `getM…
techanvil Jan 3, 2025
31cda3e
Remove the `waitForExistingTag()` action.
techanvil Jan 3, 2025
db72f9b
Remove tests for `waitForHTMLForURL()`.
techanvil Jan 3, 2025
06f851e
Remove tests for `waitForExistingTag()`.
techanvil Jan 3, 2025
1a83f18
Allow `freezeMock()` to freeze multiple requests for a given matcher.
techanvil Jan 3, 2025
e770a7e
Fix `PropertyOrWebDataStreamNotAvailableError` test.
techanvil Jan 3, 2025
4740af6
Add a test for the `getPropertySummaries()` resolver.
techanvil Jan 6, 2025
367c00c
Add tests for `getMatchingWebDataStreamByPropertyID()` and `doesWebDa…
techanvil Jan 6, 2025
62fe359
Update resolver test for `getWebContainers()`.
techanvil Jan 6, 2025
7cea708
Fix test, revert unneeded `freezeFetch()` change.
techanvil Jan 6, 2025
55d2eb9
Retrieve `getHTMLForURL()` from `resolveSelect()` outside loops.
techanvil Jan 9, 2025
32de116
Add and use a resolver for `getAMPContainers()`.
techanvil Jan 9, 2025
f01ef5d
Add a clarifying comment to the `PropertyOrWebDataStreamNotAvailableE…
techanvil Jan 9, 2025
df6be4e
Tweak comment.
techanvil Jan 9, 2025
e32c7ce
Call resolvers unconditionally.
techanvil Jan 16, 2025
c049c5d
Freeze `account-summaries` request twice in test instead of mocking i…
techanvil Jan 16, 2025
525d660
Rename the `freezeFetch()` utility's `times` option to `repeat`.
techanvil Jan 16, 2025
7b78c6f
Fix tests.
techanvil Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 3 additions & 32 deletions assets/js/googlesitekit/data/create-existing-tag-store.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ import { getExistingTagURLs, extractExistingTag } from '../../util/tag';
// Actions
const FETCH_GET_EXISTING_TAG = 'FETCH_GET_EXISTING_TAG';
const RECEIVE_GET_EXISTING_TAG = 'RECEIVE_GET_EXISTING_TAG';
const WAIT_FOR_EXISTING_TAG = 'WAIT_FOR_EXISTING_TAG';

/**
* Creates a store object that includes actions and selectors for getting existing tags.
Expand Down Expand Up @@ -89,12 +88,6 @@ export const createExistingTagStore = ( {
type: RECEIVE_GET_EXISTING_TAG,
};
},
*waitForExistingTag() {
yield {
payload: {},
type: WAIT_FOR_EXISTING_TAG,
};
},
};

const controls = {
Expand All @@ -107,13 +100,10 @@ export const createExistingTagStore = ( {
ampMode,
} );

const { getHTMLForURL } = registry.resolveSelect( CORE_SITE );

for ( const url of existingTagURLs ) {
await registry
.dispatch( CORE_SITE )
.waitForHTMLForURL( url );
const html = registry
.select( CORE_SITE )
.getHTMLForURL( url );
const html = await getHTMLForURL( url );
const tagFound = extractExistingTag( html, tagMatchers );
if ( tagFound ) {
return tagFound;
Expand All @@ -123,25 +113,6 @@ export const createExistingTagStore = ( {
return null;
}
),
[ WAIT_FOR_EXISTING_TAG ]: createRegistryControl(
( registry ) => () => {
const isExistingTagLoaded = () =>
registry.select( STORE_NAME ).getExistingTag() !==
undefined;
if ( isExistingTagLoaded() ) {
return true;
}

return new Promise( ( resolve ) => {
const unsubscribe = registry.subscribe( () => {
if ( isExistingTagLoaded() ) {
unsubscribe();
resolve();
}
} );
} );
}
),
};

const reducer = ( state = initialState, { type, payload } ) => {
Expand Down
24 changes: 0 additions & 24 deletions assets/js/googlesitekit/data/create-existing-tag-store.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,30 +118,6 @@ describe( 'createExistingTagStore store', () => {
expect( store.getState().existingTag ).toBe( existingTag );
} );
} );

describe( 'waitForExistingTag', () => {
it( 'supports asynchronous waiting for tag', async () => {
const expectedTag = 'test-tag-value';
fetchMock.getOnce(
{ query: { tagverify: '1' } },
{ body: generateHTMLWithTag( expectedTag ), status: 200 }
);

const promise = registry
.dispatch( TEST_STORE )
.waitForExistingTag();
expect( registry.select( TEST_STORE ).getExistingTag() ).toBe(
undefined
);

await promise;

expect( fetchMock ).toHaveFetchedTimes( 1 );
expect( registry.select( TEST_STORE ).getExistingTag() ).toBe(
expectedTag
);
} );
} );
} );

describe( 'selectors', () => {
Expand Down
16 changes: 0 additions & 16 deletions assets/js/googlesitekit/datastore/site/html.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,22 +129,6 @@ const baseActions = {
type: CHECK_FOR_SETUP_TAG,
};
},

/**
* Waits for HTML for to be resolved for the given URL.
*
* @since 1.13.0
* @private
*
* @param {string} url URL for which to fetch HTML.
* @yield {Object} Redux-style action.
*/
*waitForHTMLForURL( url ) {
yield {
payload: { url },
type: WAIT_FOR_HTML_FOR_URL,
};
},
};

const baseControls = {
Expand Down
28 changes: 0 additions & 28 deletions assets/js/googlesitekit/datastore/site/html.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,34 +93,6 @@ describe( 'core/site html', () => {
).toBe( html );
} );
} );

describe( 'waitForHTMLForURL', () => {
it( 'supports asynchronous waiting for HTML', async () => {
const url = 'https://example.com';
const html =
'<html><head><title>Example HTML</title></head><body><h1>Example HTML H1</h1></body></html>';

fetchMock.getOnce(
{ query: { tagverify: '1' } },
{ body: html, status: 200 }
);
const promise = registry
.dispatch( CORE_SITE )
.waitForHTMLForURL( url );

expect(
registry.select( CORE_SITE ).getHTMLForURL( url )
).toBe( undefined );

await promise;

expect( fetchMock ).toHaveFetchedTimes( 1 );

expect(
registry.select( CORE_SITE ).getHTMLForURL( url )
).toBe( html );
} );
} );
} );

describe( 'selectors', () => {
Expand Down
5 changes: 3 additions & 2 deletions assets/js/modules/adsense/datastore/ad-blocking-recovery.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,9 +105,10 @@ const controls = {
homeURL,
} );

const { getHTMLForURL } = registry.resolveSelect( CORE_SITE );

for ( const url of existingTagURLs ) {
await registry.dispatch( CORE_SITE ).waitForHTMLForURL( url );
const html = registry.select( CORE_SITE ).getHTMLForURL( url );
const html = await getHTMLForURL( url );
const tagFound = extractExistingTag(
html,
adBlockingRecoveryTagMatcher
Expand Down
26 changes: 24 additions & 2 deletions .../modules/analytics-4/components/settings/PropertyOrWebDataStreamNotAvailableError.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ describe( 'PropertyOrWebDataStreamNotAvailableError', () => {
.setMeasurementID( measurementID );
} );

it( 'should not render when properties are not loaded yet', () => {
it( 'should not render when properties are not loaded yet', async () => {
freezeFetch(
new RegExp(
'^/google-site-kit/v1/modules/analytics-4/data/account-summaries'
Expand All @@ -75,7 +75,7 @@ describe( 'PropertyOrWebDataStreamNotAvailableError', () => {
propertyIDs: [ propertyID ],
} );

const { container } = render(
const { container, waitForRegistry } = render(
<PropertyOrWebDataStreamNotAvailableError
hasModuleAccess
isDisabled={ false }
Expand All @@ -84,6 +84,28 @@ describe( 'PropertyOrWebDataStreamNotAvailableError', () => {
);

expect( container ).toBeEmptyDOMElement();

/*
* We need to mock the `GET:account-summaries` request again here, as it will be called a
* second time while resolvers are run post-render. This is due to the initial endpoint
* response being frozen, meaning the `getAccountSummaries()` resolver is unable to resolve
* itself the first time around, and will be called again when `getAccountSummaries()` is
* re-selected by another resolver.
*
* In practice, this is not an issue because the `PropertyOrWebDataStreamNotAvailableError`
* component is not rendered until the account summaries are already loaded.
*/
fetchMock.getOnce(
new RegExp(
'^/google-site-kit/v1/modules/analytics-4/data/account-summaries'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this intentionally defined after render? I don't think we could be sure it would work this way.

Copy link
Collaborator Author

@techanvil techanvil Jan 9, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is, yup. It was a bit of an odd one that took some digging to figure out what was going on.

I've added a clarifying comment, basically it's a side effect of adding the resolver for getPropertySummaries() which resolves getAccountSummaries().

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @techanvil – is it important that the resolver finishes? It seems to me that it would be more appropriate to update the fetch mock for the same endpoint above to freeze fetch "manually" using fetchMock.get rather than once so that it would handle both requests. I think the context of the comment is useful, but even if we keep the same mock, we should define it before render. This should be possible since it supports defining multiple matchers/mocks as well as simple repetition. See https://www.wheresrhys.co.uk/fetch-mock/docs/legacy-api/Usage/cheatsheet#timing-and-repetition

Copy link
Collaborator Author

@techanvil techanvil Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not essential that the resolver finishes, and I'm happy to freeze both requests. I had initially done so, in fact, but decided to take this approach instead as I figured it spells out more clearly the precise flow of events.

However, happy to make the change and I have done so.

Although using fetchMock.get() to "manually" freeze the request works, I've taken the approach of modifying freezeFetch() to allow a specific number of requests to be frozen. This retains the semantic value of calling freezeFetch() while making the test setup more explicit.

),
{
body: fixtures.accountSummaries,
status: 200,
}
);

await waitForRegistry();
} );

it( 'should not render when Web Data Streams are not loaded yet', async () => {
Expand Down
66 changes: 26 additions & 40 deletions assets/js/modules/analytics-4/datastore/properties.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,7 @@ import { createRegistrySelector } from '@wordpress/data';
* Internal dependencies
*/
import API from 'googlesitekit-api';
import {
commonActions,
combineStores,
createRegistryControl,
} from 'googlesitekit-data';
import { commonActions, combineStores } from 'googlesitekit-data';
import { CORE_USER } from '../../../googlesitekit/datastore/user/constants';
import { CORE_SITE } from '../../../googlesitekit/datastore/site/constants';
import { CORE_MODULES } from '../../../googlesitekit/modules/datastore/constants';
Expand All @@ -53,7 +49,6 @@ import {
isValidPropertyID,
isValidPropertySelection,
} from '../utils/validation';
import { actions as webDataStreamActions } from './webdatastreams';
import { createValidatedAction } from '../../../googlesitekit/data/utils';
import { getItem, setItem } from '../../../googlesitekit/api/cache';

Expand Down Expand Up @@ -202,7 +197,6 @@ const fetchSetGoogleTagIDMismatch = createFetchStore( {
} );

// Actions
const WAIT_FOR_PROPERTY_SUMMARIES = 'WAIT_FOR_PROPERTY_SUMMARIES';
const MATCHING_ACCOUNT_PROPERTY = 'MATCHING_ACCOUNT_PROPERTY';
const SET_HAS_MISMATCHED_TAG = 'SET_HAS_MISMATCHED_GOOGLE_TAG_ID';
const SET_IS_WEBDATASTREAM_AVAILABLE = 'SET_IS_WEBDATASTREAM_AVAILABLE';
Expand Down Expand Up @@ -294,11 +288,11 @@ const baseActions = {
}
}

yield webDataStreamActions.waitForWebDataStreams( propertyID );

let webdatastream = registry
.select( MODULES_ANALYTICS_4 )
.getMatchingWebDataStreamByPropertyID( propertyID );
let webdatastream = yield commonActions.await(
registry
.resolveSelect( MODULES_ANALYTICS_4 )
.getMatchingWebDataStreamByPropertyID( propertyID )
);

if ( ! webdatastream ) {
const webdatastreams = registry
Expand Down Expand Up @@ -367,12 +361,12 @@ const baseActions = {
*matchAccountProperty( accountID ) {
const registry = yield commonActions.getRegistry();

yield baseActions.waitForPropertySummaries( accountID );

const referenceURL = registry.select( CORE_SITE ).getReferenceSiteURL();
const propertySummaries = registry
.select( MODULES_ANALYTICS_4 )
.getPropertySummaries( accountID );
const propertySummaries = yield commonActions.await(
registry
.resolveSelect( MODULES_ANALYTICS_4 )
.getPropertySummaries( accountID )
);

const property = yield baseActions.matchPropertyByURL(
( propertySummaries || [] ).map( ( { _id } ) => _id ),
Expand Down Expand Up @@ -516,18 +510,6 @@ const baseActions = {
return null;
},

/**
* Waits for property summaries to be loaded for an account.
*
* @since 1.118.0
*/
*waitForPropertySummaries() {
yield {
payload: {},
type: WAIT_FOR_PROPERTY_SUMMARIES,
};
},

/**
* Updates settings for a given measurement ID.
*
Expand Down Expand Up @@ -731,17 +713,7 @@ const baseActions = {
},
};

const baseControls = {
[ WAIT_FOR_PROPERTY_SUMMARIES ]: createRegistryControl(
( { resolveSelect } ) => {
return async () => {
await resolveSelect(
MODULES_ANALYTICS_4
).getAccountSummaries();
};
}
),
};
const baseControls = {};

function baseReducer( state, { type, payload } ) {
switch ( type ) {
Expand Down Expand Up @@ -788,6 +760,20 @@ const baseResolvers = {
yield fetchGetPropertyStore.actions.fetchGetProperty( propertyID );
}
},
*getPropertySummaries( accountID ) {
const { select, resolveSelect } = yield commonActions.getRegistry();

const summaries =
select( MODULES_ANALYTICS_4 ).getAccountSummaries( accountID );

if ( summaries === undefined ) {
yield commonActions.await(
resolveSelect( MODULES_ANALYTICS_4 ).getAccountSummaries(
accountID
)
);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can and should simplify this to be unconditional since there's no downside to calling the selector here. This kind of guard is only really necessary around fetch actions, but the WP data resolver infra will only run once regardless of when this is called so it should be perfectly safe to do.

Also, there could be a situation where the underlying resolver is invalidated where, using the current logic here would prevent it from running when it should.

Copy link
Collaborator Author

@techanvil techanvil Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @aaemnnosttv, it does make sense to unconditionally call the resolver, and I've applied that change.

However, unless I've missed something, there are scenarios where a resolver can be called twice, as mentioned in my comment on the PropertyOrWebDataStreamNotAvailableError test in this PR.

site-kit-wp/assets/js/modules/analytics-4/components/settings/PropertyOrWebDataStreamNotAvailableError.test.js

Lines 88 to 97 in df6be4e

/*
* We need to mock the `GET:account-summaries` request again here, as it will be called a
* second time while resolvers are run post-render. This is due to the initial endpoint
* response being frozen, meaning the `getAccountSummaries()` resolver is unable to resolve
* itself the first time around, and will be called again when `getAccountSummaries()` is
* re-selected by another resolver.
*
* In practice, this is not an issue because the `PropertyOrWebDataStreamNotAvailableError`
* component is not rendered until the account summaries are already loaded.
*/

This can be verified in a simple manner by isolating that test and adding a log line in the *getAccountSummaries() resolver, which will log twice for the test run.

diff --git a/assets/js/modules/analytics-4/components/settings/PropertyOrWebDataStreamNotAvailableError.test.js b/assets/js/modules/analytics-4/components/settings/PropertyOrWebDataStreamNotAvailableError.test.js
index 14be657ce4..26a2837a95 100644
--- a/assets/js/modules/analytics-4/components/settings/PropertyOrWebDataStreamNotAvailableError.test.js
+++ b/assets/js/modules/analytics-4/components/settings/PropertyOrWebDataStreamNotAvailableError.test.js
@@ -62,7 +62,7 @@ describe( 'PropertyOrWebDataStreamNotAvailableError', () => {
 			.setMeasurementID( measurementID );
 	} );
 
-	it( 'should not render when properties are not loaded yet', async () => {
+	it.only( 'should not render when properties are not loaded yet', async () => {
 		freezeFetch(
 			new RegExp(
 				'^/google-site-kit/v1/modules/analytics-4/data/account-summaries'
diff --git a/assets/js/modules/analytics-4/datastore/accounts.js b/assets/js/modules/analytics-4/datastore/accounts.js
index 473dd06b07..782be3271f 100644
--- a/assets/js/modules/analytics-4/datastore/accounts.js
+++ b/assets/js/modules/analytics-4/datastore/accounts.js
@@ -338,8 +338,14 @@ const baseReducer = createReducer( ( state, { type } ) => {
 	}
 } );
 
+// Avoid console.log in tests.
+const log = ( ...args ) =>
+	process.stdout.write( args.map( JSON.stringify ).join( ' ' ) + '\n' );
+
 const baseResolvers = {
 	*getAccountSummaries() {
+		log( 'getAccountSummaries resolver' );
+
 		const registry = yield commonActions.getRegistry();
 		let nextPageToken = '';
 		const summaries = registry

I don't think this means we need to change anything, just pointing it out as a bit of unexpected behaviour to be aware of.

}
},
*getPropertyCreateTime() {
const registry = yield commonActions.getRegistry();
// Ensure settings are available to select.
Expand Down
30 changes: 30 additions & 0 deletions assets/js/modules/analytics-4/datastore/properties.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1372,6 +1372,36 @@ describe( 'modules/analytics-4 properties', () => {
} );

describe( 'getPropertySummaries', () => {
it( 'should use a resolver to make a network request', async () => {
const accountSummariesEndpoint = new RegExp(
'^/google-site-kit/v1/modules/analytics-4/data/account-summaries'
);

fetchMock.get( accountSummariesEndpoint, {
body: fixtures.accountSummaries,
status: 200,
} );

const accountID = '12345';
const initialPropertySummaries = registry
.select( MODULES_ANALYTICS_4 )
.getPropertySummaries( accountID );
expect( initialPropertySummaries ).toBeUndefined();

await untilResolved(
registry,
MODULES_ANALYTICS_4
).getPropertySummaries( accountID );

expect( fetchMock ).toHaveFetched( accountSummariesEndpoint, {
body: {
data: {
nextPageToken: '',
},
},
} );
} );

it( 'should return an empty array if no properties are present for the account', () => {
registry
.dispatch( MODULES_ANALYTICS_4 )
Expand Down
8 changes: 2 additions & 6 deletions assets/js/modules/analytics-4/datastore/settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ const store = {
};
export default store;

export async function submitChanges( { select, dispatch } ) {
export async function submitChanges( { dispatch, select, resolveSelect } ) {
let propertyID = select( MODULES_ANALYTICS_4 ).getPropertyID();
if ( propertyID === PROPERTY_CREATE ) {
const accountID = select( MODULES_ANALYTICS_4 ).getAccountID();
Expand Down Expand Up @@ -112,11 +112,7 @@ export async function submitChanges( { select, dispatch } ) {
let webDataStreamAlreadyExists = false;

if ( isValidPropertyID( propertyID ) ) {
await dispatch( MODULES_ANALYTICS_4 ).waitForWebDataStreams(
propertyID
);

webDataStreamAlreadyExists = select(
webDataStreamAlreadyExists = await resolveSelect(
MODULES_ANALYTICS_4
).doesWebDataStreamExist( propertyID, webDataStreamName );
}
Expand Down
Loading
Loading