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.

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

Framework: Add queried-data state helper utility #6395

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
30 changes: 19 additions & 11 deletions core-data/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,27 @@
import { castArray } from 'lodash';

/**
* Returns an action object used in signalling that the request for a given
* data type has been made.
* Internal dependencies
*/
import {
toggleIsRequesting,
receiveQueriedItems,
} from './queried-data';

/**
* Returns an action object used in signalling whether a request for a given
* terms of a taxonomy is in progress.
*
* @param {string} dataType Data type requested.
* @param {?string} subType Optional data sub-type.
* @param {string} taxonomy Data type requested.
* @param {?Object} query Optional terms query.
* @param {boolean} isRequesting Whether a request is in progress.
*
* @return {Object} Action object.
*/
export function setRequested( dataType, subType ) {
export function toggleIsRequestingTerms( taxonomy, query, isRequesting ) {
return {
type: 'SET_REQUESTED',
dataType,
subType,
...toggleIsRequesting( query, isRequesting ),
taxonomy,
};
}

Expand All @@ -25,15 +33,15 @@ export function setRequested( dataType, subType ) {
* for a given taxonomy.
*
* @param {string} taxonomy Taxonomy name.
* @param {?Object} query Optional terms query.
* @param {Object[]} terms Terms received.
*
* @return {Object} Action object.
*/
export function receiveTerms( taxonomy, terms ) {
export function receiveTerms( taxonomy, query, terms ) {
return {
type: 'RECEIVE_TERMS',
...receiveQueriedItems( query, terms ),
taxonomy,
terms,
};
}

Expand Down
54 changes: 54 additions & 0 deletions core-data/queried-data/actions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
/**
* Internal dependencies
*/
import {
TOGGLE_IS_REQUESTING,
RECEIVE_ITEMS,
} from './constant';

/**
* Returns an action object used in signalling whether a request for queried
* data is in progress.
*
* @param {?Object} query Optional query object.
* @param {boolean} isRequesting Whether request is in progress.
*
* @return {Object} Action object.
*/
export function toggleIsRequesting( query = {}, isRequesting ) {
return {
type: TOGGLE_IS_REQUESTING,
query,
isRequesting,
};
}

/**
* Returns an action object used in signalling that items have been received.
*
* @param {Array} items Items received.
*
* @return {Object} Action object.
*/
export function receiveItems( items ) {
return {
type: RECEIVE_ITEMS,
items,
};
}

/**
* Returns an action object used in signalling that queried data has been
* received.
*
* @param {?Object} query Optional query object.
* @param {Array} items Queried items received.
*
* @return {Object} Action object.
*/
export function receiveQueriedItems( query = {}, items ) {
return {
...receiveItems( items ),
query,
};
}
14 changes: 14 additions & 0 deletions core-data/queried-data/constant.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/**
* Action type used in signalling that a queried items page has been received.
*
* @type {string}
*/
export const RECEIVE_ITEMS = '@@queried-data/RECEIVE_ITEMS';

/**
* Action type used in signalling that a request has started or stopped for an
* items query.
*
* @type {string}
*/
export const TOGGLE_IS_REQUESTING = '@@queried-data/TOGGLE_IS_REQUESTING';
Copy link
Contributor

Choose a reason for hiding this comment

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

Any specific reason to introduce the constants. Not that I'm strongly against them but I do wonder about having some actions defined as constant and others not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Any specific reason to introduce the constants. Not that I'm strongly against them but I do wonder about having some actions defined as constant and others not.

It was implemented in the mindset of being an independent module utility, with prefixes on the action types to avoid potential conflicts. In earlier iterations, I had the consumer listening for the particular action types, where I didn't want them to be concerned with said prefixes. Since now only the internal reducer is concerned, we could just replace them with strings. Ultimately, I don't consider these files to be part of core-data or the data module patterns whatsoever, so the conventions don't overlap as much as it might appear on the surface.

Maybe a bit premature to modularize this, but having reimplemented effectively the same thing on many separate occasions, I was motivated toward something more universal 😅

64 changes: 64 additions & 0 deletions core-data/queried-data/get-query-parts.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
/**
* Internal dependencies
*/
import { withWeakMapCache } from './utils';

/**
* An object of properties describing a specific query.
*
* @typedef {WPQueriedDataQueryParts}
*
* @property {number} page The query page (1-based index, default 1).
* @property {number} perPage Items per page for query (default 10).
* @property {string} stableKey An encoded stable string of all non-pagination
* query parameters.
*/

/**
* Given a query object, returns an object of parts, including pagination
* details (`page` and `perPage`, or default values). All other properties are
* encoded into a stable (idempotent) `stableKey` value.
*
* @param {Object} query Optional query object.
*
* @return {WPQueriedDataQueryParts} Query parts.
*/
export function getQueryParts( query ) {
/**
* @type {WPQueriedDataQueryParts}
*/
const parts = {
stableKey: '',
page: 1,
perPage: 10,
};

// Ensure stable key by sorting keys. Also more efficient for iterating.
const keys = Object.keys( query ).sort();

for ( let i = 0; i < keys.length; i++ ) {
const key = keys[ i ];
const value = query[ key ];

switch ( key ) {
case 'page':
case 'perPage':
parts[ key ] = value;
break;

default:
// While it's not required to be one, for simplicity's sake
// mimic querystring encoding for stable key.
parts.stableKey += (
( parts.stableKey ? '&' : '' ) +
encodeURIComponent( key ) +
'=' +
encodeURIComponent( value )
);
}
}

return parts;
}

export default withWeakMapCache( getQueryParts );
5 changes: 5 additions & 0 deletions core-data/queried-data/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export * from './constant';
export * from './actions';
export * from './selectors';
export { default as reducer } from './reducer';
export { default as getQueryParts } from './get-query-parts';
146 changes: 146 additions & 0 deletions core-data/queried-data/reducer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
/**
* External dependencies
*/
import { combineReducers } from 'redux';
import { keyBy, map, flowRight, merge } from 'lodash';

/**
* Internal dependencies
*/
import {
ifMatchingAction,
replaceAction,
onSubKey,
} from './utils';
import {
RECEIVE_ITEMS,
TOGGLE_IS_REQUESTING,
} from './constant';
import getQueryParts from './get-query-parts';

/**
* Returns a merged array of item IDs, given details of the received paginated
* items. The array is sparse-like with `undefined` entries where holes exist.
*
* @param {?Array<number>} itemIds Original item IDs (default empty array).
* @param {number[]} nextItemIds Item IDs to merge.
* @param {number} page Page of items merged.
* @param {number} perPage Number of items per page.
*
* @return {number[]} Merged array of item IDs.
*/
export function getMergedItemIds( itemIds, nextItemIds, page, perPage ) {
const nextItemIdsStartIndex = ( page - 1 ) * perPage;

// If later page has already been received, default to the larger known
// size of the existing array, else calculate as extending the existing.
const size = Math.max(
itemIds.length,
nextItemIdsStartIndex + nextItemIds.length
);

// Preallocate array since size is known.
const mergedItemIds = new Array( size );

for ( let i = 0; i < size; i++ ) {
// Preserve existing item ID except for subset of range of next items.
const isInNextItemsRange = (
i >= nextItemIdsStartIndex &&
i < nextItemIdsStartIndex + nextItemIds.length
);

mergedItemIds[ i ] = isInNextItemsRange ?
nextItemIds[ i - nextItemIdsStartIndex ] :
itemIds[ i ];
}

return mergedItemIds;
}

/**
* Reducer tracking items state, keyed by ID. Items are assumed to be normal,
* where identifiers are common across all queries.
*
* @param {Object} state Current state.
* @param {Object} action Dispatched action.
*
* @return {Object} Next state.
*/
function items( state = {}, action ) {
switch ( action.type ) {
case RECEIVE_ITEMS:
return {
...state,
...keyBy( action.items, 'id' ),
};
}

return state;
}

/**
* Reducer tracking queries state, keyed by stable query key. Each reducer
* query object includes `itemIds` and `requestingPageByPerPage`.
*
* @param {Object} state Current state.
* @param {Object} action Dispatched action.
*
* @return {Object} Next state.
*/
const queries = flowRight( [
// Limit to matching action type so we don't attempt to replace action on
// an unhandled action.
ifMatchingAction( ( action ) => 'query' in action ),

// Inject query parts into action for use both in `onSubKey` and reducer.
replaceAction( ( action ) => {
// `ifMatchingAction` still passes on initialization, where state is
// undefined and a query is not assigned. Avoid attempting to parse
// parts. `onSubKey` will omit by lack of `stableKey`.
if ( action.query ) {
return {
...action,
...getQueryParts( action.query ),
};
}

return action;
} ),

// Queries shape is shared, but keyed by query `stableKey` part. Original
// reducer tracks only a single query object.
onSubKey( 'stableKey' ),
] )( combineReducers( {
itemIds( state = null, action ) {
const { type, page, perPage } = action;

if ( type !== RECEIVE_ITEMS ) {
return state;
}

return getMergedItemIds(
state || [],
map( action.items, 'id' ),
page,
perPage
);
},
requestingPageByPerPage( state = {}, action ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it's better to track the requesting state closely to the data or better in a separate "network requests" reducer. I've used mainly the first option for a long time but lately, I tend to think that the second one might be better as it's more generic and also speaks to the fact that these requests are "optional" in an offline mode.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think it's better to track the requesting state closely to the data or better in a separate "network requests" reducer. I've used mainly the first option for a long time but lately, I tend to think that the second one might be better as it's more generic and also speaks to the fact that these requests are "optional" in an offline mode.

I thought about this a bit and while I tend to agree that requests doesn't need to be tied so deeply to the items / queries, in practice if we try to separate them we'd end up duplicating much of the same logic anyways. As implemented, they're at least separate so far as being their own individual reducers (earlier iterations were combined into one). The behavior of getQueryParts relies on the fact that we generate a stable key which does not include pagination details. To track separately, we'd need a stable key with the pagination information. I see a few options here:

  • Manually concatenate page and perPage back into the stable key string in requests state
  • Replace getQueryParts with a simpler getStableKey, and for cases where we need to omit pagination details, do so before passing the object to getStableKey
  • Avoid building in pagination awareness into queried-data altogether. Just treat each query as distinct from one another, even on pagination arguments.

On reflection, the last of these is tempting, and could simplify things overall. The idea with the current implementation is that queries are considered the same ignoring pagination information.

There's also the question of: How much does "queried-data" make sense as a thing separate from the REST API? In your offline optional requests system you have in mind, would we still have concepts of querying by ?order=desc&page=2 ?

const { type, page, perPage, isRequesting } = action;

if ( type !== TOGGLE_IS_REQUESTING ) {
return state;
}

return merge( {}, state, {
[ perPage ]: {
[ page ]: isRequesting,
},
} );
},
} ) );

export default combineReducers( {
items,
queries,
} );
Loading