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 all 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
19 changes: 16 additions & 3 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,11 +62,22 @@ 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 () => {
/*
* We need to freeze the `GET:account-summaries` request twice 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.
*/
freezeFetch(
new RegExp(
'^/google-site-kit/v1/modules/analytics-4/data/account-summaries'
)
),
{ repeat: 2 }
);

registry
Expand All @@ -75,7 +86,7 @@ describe( 'PropertyOrWebDataStreamNotAvailableError', () => {
propertyIDs: [ propertyID ],
} );

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

expect( container ).toBeEmptyDOMElement();

await waitForRegistry();
} );

it( 'should not render when Web Data Streams are not loaded yet', async () => {
Expand Down
3 changes: 2 additions & 1 deletion assets/js/modules/analytics-4/components/setup/SetupForm.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,7 +513,8 @@ describe( 'SetupForm', () => {
triggerChange( getByRole );
} );

await waitForRegistry();
// An additional wait is required in order for all resolvers to finish.
await act( waitForDefaultTimeouts );
Comment on lines +516 to +517
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be more appropriate to use a secondary waitForRegistry here instead of an arbitrary timeout. We can always come back to it if needed though.

e.g.

diff --git a/assets/js/modules/analytics-4/components/setup/SetupForm.test.js b/assets/js/modules/analytics-4/components/setup/SetupForm.test.js
index 05ac53f02a..b9e96ea43d 100644
--- a/assets/js/modules/analytics-4/components/setup/SetupForm.test.js
+++ b/assets/js/modules/analytics-4/components/setup/SetupForm.test.js
@@ -20,6 +20,7 @@
 import { act, fireEvent, render } from '../../../../../../tests/js/test-utils';
 import {
 	createTestRegistry,
+	createWaitForRegistry,
 	muteFetch,
 	provideModules,
 	provideSiteInfo,
@@ -486,7 +487,7 @@ describe( 'SetupForm', () => {
 				.dispatch( MODULES_ANALYTICS_4 )
 				.selectAccount( accountID );
 
-			const { getByRole, getByLabelText, waitForRegistry } = render(
+			let { getByRole, getByLabelText, waitForRegistry } = render(
 				<SetupForm />,
 				{
 					registry,
@@ -509,12 +510,12 @@ describe( 'SetupForm', () => {
 
 			expect( isEnhancedMeasurementEnabled ).toBe( false );
 
-			act( () => {
+			// An additional wait is required in order for all resolvers to finish.
+			await act( async () => {
+				waitForRegistry = createWaitForRegistry( registry );
 				triggerChange( getByRole );
+				await waitForRegistry();
 			} );
-
-			// An additional wait is required in order for all resolvers to finish.
-			await act( waitForDefaultTimeouts );
 		} );
 
 		it( 'should revert the enhanced measurement from off to on', () => {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @aaemnnosttv - I had initially tried awaiting for the existing waitForRegistry() again, which didn't work - I hadn't considered created a new one. That looks like a useful pattern. We could apply it to other existing cases within this test file and no doubt in numerous other places in the codebase too.

site-kit-wp/assets/js/modules/analytics-4/components/setup/SetupForm.test.js

Lines 213 to 214 in 938a850

// An additional wait is required in order for all resolvers to finish.
await act( waitForDefaultTimeouts );

site-kit-wp/assets/js/modules/analytics-4/components/setup/SetupForm.test.js

Lines 314 to 317 in 938a850

// An additional wait is required in order for all resolvers to finish.
await act( async () => {
await waitForDefaultTimeouts();
} );

site-kit-wp/assets/js/modules/analytics-4/components/setup/SetupForm.test.js

Lines 326 to 329 in 938a850

// An additional wait is required in order for all resolvers to finish.
await act( async () => {
await waitForDefaultTimeouts();
} );

} );

it( 'should revert the enhanced measurement from off to on', () => {
Expand Down
61 changes: 21 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,15 @@ const baseResolvers = {
yield fetchGetPropertyStore.actions.fetchGetProperty( propertyID );
}
},
*getPropertySummaries( accountID ) {
const { resolveSelect } = yield commonActions.getRegistry();

yield commonActions.await(
resolveSelect( MODULES_ANALYTICS_4 ).getAccountSummaries(
accountID
)
);
},
*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