diff --git a/packages/core-data/src/selectors.ts b/packages/core-data/src/selectors.ts index f242f757d4becd..b6b36fad2ee934 100644 --- a/packages/core-data/src/selectors.ts +++ b/packages/core-data/src/selectors.ts @@ -20,6 +20,7 @@ import { getNormalizedCommaSeparable, isRawAttribute, setNestedValue, + isNumericID, } from './utils'; import type * as ET from './entity-types'; import type { UndoManager } from '@wordpress/undo-manager'; @@ -95,6 +96,13 @@ type Optional< T > = T | undefined; */ type GetRecordsHttpQuery = Record< string, any >; +/** + * Arguments for EntityRecord selectors. + */ +type EntityRecordArgs = + | [ string, string, EntityRecordKey ] + | [ string, string, EntityRecordKey, GetRecordsHttpQuery ]; + /** * Shared reference to an empty object for cases where it is important to avoid * returning a new object reference on every invocation, as in a connected or @@ -292,6 +300,7 @@ export interface GetEntityRecord { key: EntityRecordKey, query?: GetRecordsHttpQuery ) => EntityRecord | undefined; + __unstableNormalizeArgs?: ( args: EntityRecordArgs ) => EntityRecordArgs; } /** @@ -365,6 +374,24 @@ export const getEntityRecord = createSelector( } ) as GetEntityRecord; +/** + * Normalizes `recordKey`s that look like numeric IDs to numbers. + * + * @param args EntityRecordArgs the selector arguments. + * @return EntityRecordArgs the normalized arguments. + */ +getEntityRecord.__unstableNormalizeArgs = ( + args: EntityRecordArgs +): EntityRecordArgs => { + const newArgs = [ ...args ] as EntityRecordArgs; + const recordKey = newArgs?.[ 2 ]; + + // If recordKey looks to be a numeric ID then coerce to number. + newArgs[ 2 ] = isNumericID( recordKey ) ? Number( recordKey ) : recordKey; + + return newArgs; +}; + /** * Returns the Entity's record object by key. Doesn't trigger a resolver nor requests the entity records from the API if the entity record isn't available in the local state. * diff --git a/packages/core-data/src/test/selectors.js b/packages/core-data/src/test/selectors.js index 61c0621b6e5e49..b7c583098c4a5c 100644 --- a/packages/core-data/src/test/selectors.js +++ b/packages/core-data/src/test/selectors.js @@ -28,6 +28,29 @@ describe.each( [ [ getEntityRecord ], [ __experimentalGetEntityRecordNoResolver ], ] )( '%p', ( selector ) => { + describe( 'normalizing Post ID passed as recordKey', () => { + it( 'normalizes any Post ID recordKey argument to a Number via `__unstableNormalizeArgs` method', async () => { + const normalized = getEntityRecord.__unstableNormalizeArgs( [ + 'postType', + 'some_post', + '123', + ] ); + expect( normalized ).toEqual( [ 'postType', 'some_post', 123 ] ); + } ); + + it( 'does not normalize recordKey argument unless it is a Post ID', async () => { + const normalized = getEntityRecord.__unstableNormalizeArgs( [ + 'postType', + 'some_post', + 'i-am-a-slug-with-a-number-123', + ] ); + expect( normalized ).toEqual( [ + 'postType', + 'some_post', + 'i-am-a-slug-with-a-number-123', + ] ); + } ); + } ); it( 'should return undefined for unknown entity kind, name', () => { const state = deepFreeze( { entities: { diff --git a/packages/core-data/src/utils/index.js b/packages/core-data/src/utils/index.js index f37efe6eee7fda..d4d491fab9827d 100644 --- a/packages/core-data/src/utils/index.js +++ b/packages/core-data/src/utils/index.js @@ -8,3 +8,4 @@ export { default as withWeakMapCache } from './with-weak-map-cache'; export { default as isRawAttribute } from './is-raw-attribute'; export { default as setNestedValue } from './set-nested-value'; export { default as getNestedValue } from './get-nested-value'; +export { default as isNumericID } from './is-numeric-id'; diff --git a/packages/core-data/src/utils/is-numeric-id.js b/packages/core-data/src/utils/is-numeric-id.js new file mode 100644 index 00000000000000..1bc05575d0aeda --- /dev/null +++ b/packages/core-data/src/utils/is-numeric-id.js @@ -0,0 +1,10 @@ +/** + * Checks argument to determine if it's a numeric ID. + * For example, '123' is a numeric ID, but '123abc' is not. + * + * @param {any} id the argument to determine if it's a numeric ID. + * @return {boolean} true if the string is a numeric ID, false otherwise. + */ +export default function isNumericID( id ) { + return /^\s*\d+\s*$/.test( id ); +} diff --git a/packages/core-data/src/utils/test/is-numeric-id.js b/packages/core-data/src/utils/test/is-numeric-id.js new file mode 100644 index 00000000000000..ac879887742561 --- /dev/null +++ b/packages/core-data/src/utils/test/is-numeric-id.js @@ -0,0 +1,22 @@ +/** + * Internal dependencies + */ +import isNumericID from '../is-numeric-id'; + +describe( 'isNumericID', () => { + test.each( [ + [ true, '12345' ], + [ true, '42' ], + [ true, ' 42 ' ], + [ false, 'abc123' ], + [ false, '123abc' ], + [ false, '' ], + [ false, '123-abc' ], + [ false, 'abc-123' ], + [ false, '42-test-123' ], + [ false, '3.42' ], + [ true, 123 ], + ] )( `should return %s for input "%s"`, ( expected, input ) => { + expect( isNumericID( input ) ).toBe( expected ); + } ); +} ); diff --git a/packages/data/README.md b/packages/data/README.md index 96115e734266ef..7a12a913e661e2 100644 --- a/packages/data/README.md +++ b/packages/data/README.md @@ -1127,6 +1127,86 @@ _Returns_ - `boolean`: Whether resolution is in progress. +### Normalizing Selector Arguments + +In specific circumstances it may be necessary to normalize the arguments passed to a given _call_ of a selector/resolver pairing. + +Each resolver has [its resolution status cached in an internal state](https://github.com/WordPress/gutenberg/blob/e244388d8669618b76c966cc33d48df9156c2db6/packages/data/src/redux-store/metadata/reducer.ts#L39) where the [key is the arguments supplied to the selector](https://github.com/WordPress/gutenberg/blob/e244388d8669618b76c966cc33d48df9156c2db6/packages/data/src/redux-store/metadata/utils.ts#L48) at _call_ time. + +For example for a selector with a single argument, the related resolver would generate a cache key of: `[ 123 ]`. + +[This cache is used to determine the resolution status of a given resolver](https://github.com/WordPress/gutenberg/blob/e244388d8669618b76c966cc33d48df9156c2db6/packages/data/src/redux-store/metadata/selectors.js#L22-L29) which is used to [avoid unwanted additional invocations of resolvers](https://github.com/WordPress/gutenberg/blob/e244388d8669618b76c966cc33d48df9156c2db6/packages/data/src/redux-store/index.js#L469-L474) (which often undertake "expensive" operations such as network requests). + +As a result it's important that arguments remain _consistent_ when calling the selector. For example, by _default_ these two calls will not be cached using the same key, even though they are likely identical: + +```js +// Arg as number +getSomeDataById( 123 ); + +// Arg as string +getSomeDataById( '123' ); +``` + +This is an opportunity to utilize the `__unstableNormalizeArgs` property to guarantee consistency by protecting callers from passing incorrect types. + +#### Example + +The _3rd_ argument of the following selector is intended to be a `Number`: + +```js +const getItemsSelector = ( name, type, id ) => { + return state.items[ name ][ type ][ id ] || null; +}; +``` + +However, it is possible that the `id` parameter will be passed as a `String`. In this case, the `__unstableNormalizeArgs` method (property) can be defined on the _selector_ to coerce the arguments to the desired type even if they are provided "incorrectly": + +```js +// Define normalization method. +getItemsSelector.__unstableNormalizeArgs = ( args ) { + // "id" argument at the 2nd index + if (args[2] && typeof args[2] === 'string' ) { + args[2] === Number(args[2]); + } + + return args; +} +``` + +With this in place the following code will behave consistently: + +```js +const getItemsSelector = ( name, type, id ) => { + // here 'id' is now guaranteed to be a number. + return state.items[ name ][ type ][ id ] || null; +}; + +const getItemsResolver = ( name, type, id ) => { + // 'id' is also guaranteed to be a number in the resolver. + return {}; +}; + +registry.registerStore( 'store', { + // ... + selectors: { + getItems: getItemsSelector, + }, + resolvers: { + getItems: getItemsResolver, + }, +} ); + +// Call with correct number type. +registry.select( 'store' ).getItems( 'foo', 'bar', 54 ); + +// Call with the wrong string type, **but** here we have avoided an +// wanted resolver call because '54' is guaranteed to have been +// coerced to a number by the `__unstableNormalizeArgs` method. +registry.select( 'store' ).getItems( 'foo', 'bar', '54' ); +``` + +Ensuring consistency of arguments for a given selector call is [an important optimization to help improve performance in the data layer](https://github.com/WordPress/gutenberg/pull/52120). However, this type of problem can be usually be avoided by ensuring selectors don't use variable types for their arguments. + ## Going further - [What is WordPress Data?](https://unfoldingneurons.com/2020/what-is-wordpress-data/) diff --git a/packages/data/src/redux-store/index.js b/packages/data/src/redux-store/index.js index 3a4b03b23001be..e8394087215428 100644 --- a/packages/data/src/redux-store/index.js +++ b/packages/data/src/redux-store/index.js @@ -235,11 +235,19 @@ export default function createReduxStore( key, options ) { selector.registry = registry; } const boundSelector = ( ...args ) => { + args = normalize( selector, args ); const state = store.__unstableOriginalGetState(); return selector( state.root, ...args ); }; + // Expose normalization method on the bound selector + // in order that it can be called when fullfilling + // the resolver. + boundSelector.__unstableNormalizeArgs = + selector.__unstableNormalizeArgs; + const resolver = resolvers[ selectorName ]; + if ( ! resolver ) { boundSelector.hasResolver = false; return boundSelector; @@ -254,10 +262,24 @@ export default function createReduxStore( key, options ) { ); } - function bindMetadataSelector( selector ) { + function bindMetadataSelector( metaDataSelector ) { const boundSelector = ( ...args ) => { const state = store.__unstableOriginalGetState(); - return selector( state.metadata, ...args ); + + const originalSelectorName = args && args[ 0 ]; + const originalSelectorArgs = args && args[ 1 ]; + const targetSelector = + options?.selectors?.[ originalSelectorName ]; + + // Normalize the arguments passed to the target selector. + if ( originalSelectorName && targetSelector ) { + args[ 1 ] = normalize( + targetSelector, + originalSelectorArgs + ); + } + + return metaDataSelector( state.metadata, ...args ); }; boundSelector.hasResolver = false; return boundSelector; @@ -604,9 +626,29 @@ function mapSelectorWithResolver( } const selectorResolver = ( ...args ) => { + args = normalize( selector, args ); fulfillSelector( args ); return selector( ...args ); }; selectorResolver.hasResolver = true; return selectorResolver; } + +/** + * Applies selector's normalization function to the given arguments + * if it exists. + * + * @param {Object} selector The selector potentially with a normalization method property. + * @param {Array} args selector arguments to normalize. + * @return {Array} Potentially normalized arguments. + */ +function normalize( selector, args ) { + if ( + selector.__unstableNormalizeArgs && + typeof selector.__unstableNormalizeArgs === 'function' && + args?.length + ) { + return selector.__unstableNormalizeArgs( args ); + } + return args; +} diff --git a/packages/data/src/redux-store/metadata/test/selectors.js b/packages/data/src/redux-store/metadata/test/selectors.js index 11eba0301e8c55..faf9da3a169853 100644 --- a/packages/data/src/redux-store/metadata/test/selectors.js +++ b/packages/data/src/redux-store/metadata/test/selectors.js @@ -3,6 +3,8 @@ */ import { createRegistry } from '@wordpress/data'; +const getFooSelector = ( state ) => state; + const testStore = { reducer: ( state = null, action ) => { if ( action.type === 'RECEIVE' ) { @@ -12,7 +14,7 @@ const testStore = { return state; }, selectors: { - getFoo: ( state ) => state, + getFoo: getFooSelector, }, }; @@ -55,7 +57,7 @@ describe( 'getIsResolving', () => { expect( result ).toBe( true ); } ); - it( 'should normalize args ard return the right value', () => { + it( 'should normalize args and return the right value', () => { registry.dispatch( 'testStore' ).startResolution( 'getFoo', [] ); const { getIsResolving } = registry.select( 'testStore' ); @@ -443,3 +445,37 @@ describe( 'countSelectorsByStatus', () => { expect( result1 ).not.toBe( result2 ); } ); } ); + +describe( 'Selector arguments normalization', () => { + let registry; + beforeEach( () => { + registry = createRegistry(); + registry.registerStore( 'testStore', testStore ); + } ); + + it( 'should call normalization method on target selector if exists', () => { + const normalizationFunction = jest.fn( ( args ) => { + return args.map( Number ); + } ); + getFooSelector.__unstableNormalizeArgs = normalizationFunction; + + registry.dispatch( 'testStore' ).startResolution( 'getFoo', [ 123 ] ); + const { getIsResolving, hasStartedResolution, hasFinishedResolution } = + registry.select( 'testStore' ); + + expect( getIsResolving( 'getFoo', [ '123' ] ) ).toBe( true ); + expect( normalizationFunction ).toHaveBeenCalledWith( [ '123' ] ); + + expect( hasStartedResolution( 'getFoo', [ '123' ] ) ).toBe( true ); + expect( normalizationFunction ).toHaveBeenCalledWith( [ '123' ] ); + + expect( normalizationFunction ).toHaveBeenCalledTimes( 2 ); + + registry.dispatch( 'testStore' ).finishResolution( 'getFoo', [ 123 ] ); + + expect( hasFinishedResolution( 'getFoo', [ '123' ] ) ).toBe( true ); + expect( normalizationFunction ).toHaveBeenCalledWith( [ '123' ] ); + + getFooSelector.__unstableNormalizeArgs = undefined; + } ); +} ); diff --git a/packages/data/src/redux-store/test/index.js b/packages/data/src/redux-store/test/index.js index daca128480daeb..1251051c4c9d9a 100644 --- a/packages/data/src/redux-store/test/index.js +++ b/packages/data/src/redux-store/test/index.js @@ -286,3 +286,75 @@ describe( 'resolveSelect', () => { ] ); } ); } ); + +describe( 'normalizing args', () => { + it( 'should call the __unstableNormalizeArgs method of the selector for both the selector and the resolver', async () => { + const registry = createRegistry(); + const selector = () => {}; + + const normalizingFunction = jest.fn( ( ...args ) => args ); + + selector.__unstableNormalizeArgs = normalizingFunction; + + registry.registerStore( 'store', { + reducer: () => {}, + selectors: { + getItems: selector, + }, + resolvers: { + getItems: () => 'items', + }, + } ); + registry.select( 'store' ).getItems( 'foo', 'bar' ); + + expect( normalizingFunction ).toHaveBeenCalledWith( [ 'foo', 'bar' ] ); + + // Needs to be called twice: + // 1. When the selector is called. + // 2. When the resolver is fullfilled. + expect( normalizingFunction ).toHaveBeenCalledTimes( 2 ); + } ); + + it( 'should not call the __unstableNormalizeArgs method if there are no arguments passed to the selector (and thus the resolver)', async () => { + const registry = createRegistry(); + const selector = () => {}; + + selector.__unstableNormalizeArgs = jest.fn( ( ...args ) => args ); + + registry.registerStore( 'store', { + reducer: () => {}, + selectors: { + getItems: selector, + }, + resolvers: { + getItems: () => 'items', + }, + } ); + + // Called with no args so the __unstableNormalizeArgs method should not be called. + registry.select( 'store' ).getItems(); + + expect( selector.__unstableNormalizeArgs ).not.toHaveBeenCalled(); + } ); + + it( 'should call the __unstableNormalizeArgs method on the selectors without resolvers', async () => { + const registry = createRegistry(); + const selector = () => {}; + + selector.__unstableNormalizeArgs = jest.fn( ( ...args ) => args ); + + registry.registerStore( 'store', { + reducer: () => {}, + selectors: { + getItems: selector, + }, + } ); + + registry.select( 'store' ).getItems( 'foo', 'bar' ); + + expect( selector.__unstableNormalizeArgs ).toHaveBeenCalledWith( [ + 'foo', + 'bar', + ] ); + } ); +} );