Skip to content
This repository has been archived by the owner on Jul 12, 2024. It is now read-only.

Try: Collect async resolver calls #4444

Merged
merged 14 commits into from
Jun 4, 2020
6 changes: 2 additions & 4 deletions client/dashboard/task-list/tasks/payments/square.js
Original file line number Diff line number Diff line change
Expand Up @@ -145,16 +145,14 @@ class Square extends Component {

export default compose(
withSelect( ( select ) => {
const { getOptions, isGetOptionsRequesting } = select(
const { getOptions, isRequesting } = select(
OPTIONS_STORE_NAME
);
const options = getOptions( [
'woocommerce_square_credit_card_settings',
] );
const optionsIsRequesting = Boolean(
isGetOptionsRequesting( [
'woocommerce_square_credit_card_settings',
] )
isRequesting( 'woocommerce_square_credit_card_settings' )
);

return {
Expand Down
6 changes: 2 additions & 4 deletions client/dashboard/task-list/tasks/payments/wcpay.js
Original file line number Diff line number Diff line change
Expand Up @@ -127,16 +127,14 @@ class WCPay extends Component {

export default compose(
withSelect( ( select ) => {
const { getOptions, isGetOptionsRequesting } = select(
const { getOptions, isRequesting } = select(
OPTIONS_STORE_NAME
);
const options = getOptions( [
'woocommerce_woocommerce_payments_settings',
] );
const optionsIsRequesting = Boolean(
isGetOptionsRequesting( [
'woocommerce_woocommerce_payments_settings',
] )
isRequesting( 'woocommerce_woocommerce_payments_settings' )
);

return {
Expand Down
26 changes: 24 additions & 2 deletions client/homepage/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@
*/
import { compose } from '@wordpress/compose';
import { Suspense, lazy } from '@wordpress/element';
import { identity } from 'lodash';

/**
* WooCommerce dependencies
*/
import { Spinner } from '@woocommerce/components';
import { withOptionsHydration, OPTIONS_STORE_NAME } from '@woocommerce/data';

/**
* Internal dependencies
Expand All @@ -19,7 +21,9 @@ const ProfileWizard = lazy( () =>
import( /* webpackChunkName: "profile-wizard" */ '../profile-wizard' )
);

const Homepage = ( { profileItems, query } ) => {
const Homepage = ( { profileItems, query, blogname, blogdescription } ) => {
console.log( blogname );
console.log( blogdescription );
if ( isOnboardingEnabled() && ! profileItems.completed ) {
return (
<Suspense fallback={ <Spinner /> }>
Expand All @@ -32,6 +36,11 @@ const Homepage = ( { profileItems, query } ) => {
};

export default compose(
window.wcSettings.preloadOptions
? withOptionsHydration( {
...window.wcSettings.preloadOptions,
} )
: identity,
withSelect( ( select ) => {
if ( ! isOnboardingEnabled() ) {
return;
Expand All @@ -40,6 +49,19 @@ export default compose(
const { getProfileItems } = select( 'wc-api' );
const profileItems = getProfileItems();

return { profileItems };
const { getOption, isRequesting } = select( OPTIONS_STORE_NAME );
const blogname = getOption( 'blogname' );
const blogdescription = getOption( 'blogdescription' );

console.log(
'isRequesting blogname',
isRequesting( 'blogname' )
);
console.log(
'isRequesting blogdescription',
isRequesting( 'blogdescription' )
);

return { profileItems, blogname, blogdescription };
} )
)( Homepage );
7 changes: 4 additions & 3 deletions packages/data/src/options/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,18 @@ export function receiveOptions( options ) {
};
}

export function setIsRequesting( isRequesting ) {
export function setIsRequesting( optionName ) {
return {
type: TYPES.SET_IS_REQUESTING,
isRequesting,
optionName
};
}

export function setRequestingError( error ) {
export function setRequestingError( error, options ) {
return {
type: TYPES.SET_REQUESTING_ERROR,
error,
options
};
}

Expand Down
31 changes: 31 additions & 0 deletions packages/data/src/options/controls.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/**
* External dependencies
*/
import { controls as dataControls } from '@wordpress/data-controls';

let optionNames = [];

export const getOptionsToRequest = ( optionName ) => {
return {
type: 'GET_OPTIONS_TO_REQUEST',
optionName,
};
};

export const controls = {
...dataControls,
GET_OPTIONS_TO_REQUEST( { optionName } ) {
optionNames.push( optionName );

return new Promise( resolve => {
setTimeout( () => {
resolve( optionNames.join(',') );

// Clear option names after all resolved;
setTimeout( () => {
optionNames = [];
}, 1 )
}, 1 );
} );
},
};
2 changes: 1 addition & 1 deletion packages/data/src/options/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
*/

import { registerStore } from '@wordpress/data';
import { controls } from '@wordpress/data-controls';

/**
* Internal dependencies
Expand All @@ -12,6 +11,7 @@ import { STORE_NAME } from './constants';
import * as selectors from './selectors';
import * as actions from './actions';
import * as resolvers from './resolvers';
import { controls } from './controls';
import reducer from './reducer';

registerStore( STORE_NAME, {
Expand Down
26 changes: 21 additions & 5 deletions packages/data/src/options/reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,22 +3,35 @@
*/
import TYPES from './action-types';

const getIsRequestingObject = ( options, value ) => {
return Object.keys( options ).reduce( ( result, name ) => {
result[ name ] = value;
return result;
}, {} );
};

const optionsReducer = (
state = { isRequesting: false, isUpdating: false },
{ type, options, isRequesting, error, isUpdating }
state = { isRequesting: {}, isUpdating: false },
{ type, options, error, isUpdating, optionName }
) => {
switch ( type ) {
case TYPES.RECEIVE_OPTIONS:
state = {
...state,
...options,
isRequesting: false,
isRequesting: {
...state.isRequesting,
...getIsRequestingObject( options, false ),
},
};
break;
case TYPES.SET_IS_REQUESTING:
state = {
...state,
isRequesting,
isRequesting: {
...state.isRequesting,
[ optionName ]: true,
},
};
break;
case TYPES.SET_IS_UPDATING:
Expand All @@ -31,7 +44,10 @@ const optionsReducer = (
state = {
...state,
requestingError: error,
isRequesting: false,
isRequesting: {
...state.isRequesting,
...getIsRequestingObject( options, false ),
},
};
break;
case TYPES.SET_UPDATING_ERROR:
Expand Down
29 changes: 27 additions & 2 deletions packages/data/src/options/resolvers.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
/**
* External Dependencies
*/

import { apiFetch } from '@wordpress/data-controls';
import { getOptionsToRequest } from './controls';

/**
* Internal dependencies
Expand All @@ -19,7 +19,32 @@ export function* getOptionsWithRequest( names ) {
yield receiveOptions( results );
return results;
} catch ( error ) {
yield setRequestingError( error );
yield setRequestingError( error, names );
return error;
}
}

const fetches = {};

export function* getOption( name ) {
yield setIsRequesting( name );
const names = yield getOptionsToRequest( name );

const fetchInProgress = fetches[ names ];
if ( fetchInProgress ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could probably just eliminate the declaration of fetchInProgress here and use fetches[names] directly in the condition.

return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a heads up that I added this back in since we need to return early here.

Returning yield apiFetch( { path: url } ); will hit the options endpoint again for each request.

Copy link
Contributor

Choose a reason for hiding this comment

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

As Paul pointed out, this will cause the built in cached resolver for getOption( name ) to be marked as finished - so this effectively means you cannot depend on the wp.data resolution state for any of the options to be accurate. This could be a gotcha for folks not realizing getOption has a special resolution state.

}

const url = WC_ADMIN_NAMESPACE + '/options?options=' + names;
fetches[ names ] = true;

try {
const result = yield apiFetch( { path: url } );
yield receiveOptions( result );
} catch ( error ) {
yield setRequestingError( error, names );
}

// Delete the fetch after to allow wp data to handle cache invalidation.
delete fetches[ names ];
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is a bit ambiguous. Are you talking about wp.data handling resolver cache invalidation here? If true, then no this won't handle resolver cache invalidation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Up for rewording this comment, but I believe it's accurate. We need to delete the fetch from our local variable to allow (keyword) wp data to handle invalidation. It does not invalidate cache directly, just allows it.

If we don't remove this fetch and we want to invalidate this cache using invalidateResolution, it would never actually work since the previous promise has already resolved.

}
9 changes: 7 additions & 2 deletions packages/data/src/options/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ export const getOptions = ( state, names ) => {
}, {} );
};

export const getOption = ( state, name ) => {
return state[ name ];
};

/**
* Get options from state tree or make a request if unresolved.
*
Expand All @@ -44,9 +48,10 @@ export const getOptionsWithRequest = ( state, names ) => {
* Determine if options are being requested.
*
* @param {Object} state - Reducer state
* @param {string} optionName - Option name
*/
export const isGetOptionsRequesting = ( state ) => {
return state.isRequesting || false;
export const isRequesting = ( state, optionName ) => {
return state.isRequesting[ optionName ] || false;
};

/**
Expand Down
18 changes: 10 additions & 8 deletions packages/data/src/options/with-options-hydration.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,16 @@ export const withOptionsHydration = ( data ) => ( OriginalComponent ) => {
} = registry.dispatch( STORE_NAME );
const names = Object.keys( dataRef.current );

if (
! isResolving( 'getOptionsWithRequest', names ) &&
! hasFinishedResolution( 'getOptionsWithRequest', names )
) {
startResolution( 'getOptionsWithRequest', names );
receiveOptions( dataRef.current );
finishResolution( 'getOptionsWithRequest', names );
}
names.forEach( ( name ) => {
if (
! isResolving( 'getOption', [ name ] ) &&
! hasFinishedResolution( 'getOption', [ name ] )
) {
startResolution( 'getOption', [ name ] );
receiveOptions( { [ name ]: dataRef.current[ name ] } );
finishResolution( 'getOption', [ name ] );
}
} );
}, [] );

return <OriginalComponent { ...props } />;
Expand Down