Refactor waitFor* actions with resolveSelect.
#9978
Conversation
…etPropertySummaries()`.
…atchingWebDataStreamByPropertyID()` and `doesWebDataStreamExist()`.
…taStreamExist()` resolvers.
techanvil
force-pushed
the
enhancement/9008-remove-waitfor-actions
branch
from
dfc9788 to
7cea708
Compare
|
Build files for 7b78c6f have been deleted. |
|
Size Change: -1.21 kB (-0.06%) Total Size: 1.98 MB
ℹ️ View Unchanged
|
|
LGTM 💯 Moving this to MR. |
There was a problem hiding this comment.
Thanks @techanvil – this looks solid overall. Looks like I missed the detail about AMP containers in the IB but it should be quick to address. A few other questions/suggestions below as well.
| fetchMock.getOnce( | ||
| new RegExp( | ||
| '^/google-site-kit/v1/modules/analytics-4/data/account-summaries' |
There was a problem hiding this comment.
Is this intentionally defined after render? I don't think we could be sure it would work this way.
There was a problem hiding this comment.
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().
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks @techanvil ! This is almost there, just a few comments left to address.
| fetchMock.getOnce( | ||
| new RegExp( | ||
| '^/google-site-kit/v1/modules/analytics-4/data/account-summaries' |
There was a problem hiding this comment.
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
| const summaries = | ||
| select( MODULES_ANALYTICS_4 ).getAccountSummaries( accountID ); | ||
|
|
||
| if ( summaries === undefined ) { | ||
| yield commonActions.await( | ||
| resolveSelect( MODULES_ANALYTICS_4 ).getAccountSummaries( | ||
| accountID | ||
| ) | ||
| ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.
| if ( ! isValidPropertyID( propertyID ) ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
How about we move this guard to getWebDataStreams below? Then this function can resolve-select that here unconditionally similar to my comment above.
There was a problem hiding this comment.
Sounds good - just to be clear though, did you mean to move it to the *getWebDataStreams() resolver? That's what I have done, but arguably it would be more consistent to move it to an invariant() check in the getWebDataStreams() selector.
I was a bit hesitant to add the invariant() call, because it could materially change the current execution path and user flow in some edge case out there, but I'm interested to get your thoughts on it.
There was a problem hiding this comment.
Yep, that's what I meant (the resolver). It just doesn't make sense to fetch for an invalid property ID 👍
| // An additional wait is required in order for all resolvers to finish. | ||
| await act( waitForDefaultTimeouts ); |
There was a problem hiding this comment.
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', () => {There was a problem hiding this comment.
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
site-kit-wp/assets/js/modules/analytics-4/components/setup/SetupForm.test.js
Lines 314 to 317 in 938a850
site-kit-wp/assets/js/modules/analytics-4/components/setup/SetupForm.test.js
Lines 326 to 329 in 938a850
Summary
Addresses issue:
waitFor*actions withresolveSelect#9008Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist