From 6bcec6e121459665ebcb4e3aac05f650a2969faa Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Thu, 29 Jun 2023 12:23:38 +0100 Subject: [PATCH 01/33] Normalize --- .../data/src/redux-store/metadata/utils.ts | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/packages/data/src/redux-store/metadata/utils.ts b/packages/data/src/redux-store/metadata/utils.ts index a6e7babca76cb9..a347bfbe67ff04 100644 --- a/packages/data/src/redux-store/metadata/utils.ts +++ b/packages/data/src/redux-store/metadata/utils.ts @@ -38,6 +38,24 @@ export const onSubKey = }; }; +function isNumeric( n ) { + return ! isNaN( parseFloat( n ) ) && isFinite( n ); +} + +// write a function to parse an array of args and convert numeric strings to true numbers +// using the isNumeric function +function normalizeNumeric( args ) { + return args?.map( ( arg ) => { + if ( Array.isArray( arg ) ) { + return normalizeNumeric( arg ); + } + if ( typeof arg === 'string' && isNumeric( arg ) ) { + return parseFloat( arg ); + } + return arg; + } ); +} + /** * Normalize selector argument array by defaulting `undefined` value to an empty array * and removing trailing `undefined` values. @@ -46,6 +64,8 @@ export const onSubKey = * @return Normalized state key array */ export function selectorArgsToStateKey( args: unknown[] | null | undefined ) { + args = normalizeNumeric( args ); + if ( args === undefined || args === null ) { return []; } From 9f8f093472be403fde624a1186344ed178d4974d Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Thu, 29 Jun 2023 13:24:58 +0100 Subject: [PATCH 02/33] Add optional method property to normalize arguments before usage --- packages/core-data/src/resolvers.js | 16 +++++++++++++++ packages/data/src/redux-store/index.js | 3 +++ .../data/src/redux-store/metadata/utils.ts | 20 ------------------- 3 files changed, 19 insertions(+), 20 deletions(-) diff --git a/packages/core-data/src/resolvers.js b/packages/core-data/src/resolvers.js index 07e9cd98cb5ec3..80d1c2d3564e52 100644 --- a/packages/core-data/src/resolvers.js +++ b/packages/core-data/src/resolvers.js @@ -172,6 +172,22 @@ export const getEntityRecord = } }; +function isNumeric( n ) { + return ! isNaN( parseFloat( n ) ) && isFinite( n ); +} + +getEntityRecord.normalizeArgs = ( args ) => { + let key = args && args[ 2 ]; + + // If key is numeric, assume it's an ID and coerce to number. + if ( key && typeof key === 'string' && isNumeric( key ) ) { + key = Number( key ); + } + + args[ 2 ] = key; + return args; +}; + /** * Requests an entity's record from the REST API. */ diff --git a/packages/data/src/redux-store/index.js b/packages/data/src/redux-store/index.js index 3a4b03b23001be..d07e5ca048d584 100644 --- a/packages/data/src/redux-store/index.js +++ b/packages/data/src/redux-store/index.js @@ -604,6 +604,9 @@ function mapSelectorWithResolver( } const selectorResolver = ( ...args ) => { + if ( resolver.normalizeArgs ) { + args = resolver.normalizeArgs( args ); + } fulfillSelector( args ); return selector( ...args ); }; diff --git a/packages/data/src/redux-store/metadata/utils.ts b/packages/data/src/redux-store/metadata/utils.ts index a347bfbe67ff04..a6e7babca76cb9 100644 --- a/packages/data/src/redux-store/metadata/utils.ts +++ b/packages/data/src/redux-store/metadata/utils.ts @@ -38,24 +38,6 @@ export const onSubKey = }; }; -function isNumeric( n ) { - return ! isNaN( parseFloat( n ) ) && isFinite( n ); -} - -// write a function to parse an array of args and convert numeric strings to true numbers -// using the isNumeric function -function normalizeNumeric( args ) { - return args?.map( ( arg ) => { - if ( Array.isArray( arg ) ) { - return normalizeNumeric( arg ); - } - if ( typeof arg === 'string' && isNumeric( arg ) ) { - return parseFloat( arg ); - } - return arg; - } ); -} - /** * Normalize selector argument array by defaulting `undefined` value to an empty array * and removing trailing `undefined` values. @@ -64,8 +46,6 @@ function normalizeNumeric( args ) { * @return Normalized state key array */ export function selectorArgsToStateKey( args: unknown[] | null | undefined ) { - args = normalizeNumeric( args ); - if ( args === undefined || args === null ) { return []; } From 355ee98ce1a1536793d98c7afa5deb20e555eb48 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Thu, 29 Jun 2023 15:31:52 +0100 Subject: [PATCH 03/33] Check for ID-like keys rather than numeric things MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Accounts for cases such as ‘this-is-a-slug-1234’ --- packages/core-data/src/resolvers.js | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/packages/core-data/src/resolvers.js b/packages/core-data/src/resolvers.js index 80d1c2d3564e52..6a7e14ac215b48 100644 --- a/packages/core-data/src/resolvers.js +++ b/packages/core-data/src/resolvers.js @@ -172,15 +172,33 @@ export const getEntityRecord = } }; -function isNumeric( n ) { - return ! isNaN( parseFloat( n ) ) && isFinite( n ); +function isNumericID( str ) { + // Remove leading/trailing whitespace + str = str.trim(); + + // Check if the string is empty + if ( str.length === 0 ) { + return false; + } + + // Check if the string is a number + if ( ! isNaN( str ) ) { + return true; + } + + // Check if the string contains only numeric characters + if ( /^\d+$/.test( str ) ) { + return true; + } + + return false; } getEntityRecord.normalizeArgs = ( args ) => { let key = args && args[ 2 ]; // If key is numeric, assume it's an ID and coerce to number. - if ( key && typeof key === 'string' && isNumeric( key ) ) { + if ( key && typeof key === 'string' && isNumericID( key ) ) { key = Number( key ); } From b0598a2b6ab96673e7aafc0859093f0f8e0b3f87 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Fri, 30 Jun 2023 09:21:19 +0100 Subject: [PATCH 04/33] Extract util and update to use suggestion from Code Review --- packages/core-data/src/resolvers.js | 28 ++++--------------- packages/core-data/src/utils/index.js | 1 + packages/core-data/src/utils/is-numeric-id.js | 10 +++++++ 3 files changed, 16 insertions(+), 23 deletions(-) create mode 100644 packages/core-data/src/utils/is-numeric-id.js diff --git a/packages/core-data/src/resolvers.js b/packages/core-data/src/resolvers.js index 6a7e14ac215b48..4a26541b664497 100644 --- a/packages/core-data/src/resolvers.js +++ b/packages/core-data/src/resolvers.js @@ -15,7 +15,11 @@ import apiFetch from '@wordpress/api-fetch'; */ import { STORE_NAME } from './name'; import { getOrLoadEntitiesConfig, DEFAULT_ENTITY_KEY } from './entities'; -import { forwardResolver, getNormalizedCommaSeparable } from './utils'; +import { + forwardResolver, + getNormalizedCommaSeparable, + isNumericID, +} from './utils'; import { getSyncProvider } from './sync'; /** @@ -172,28 +176,6 @@ export const getEntityRecord = } }; -function isNumericID( str ) { - // Remove leading/trailing whitespace - str = str.trim(); - - // Check if the string is empty - if ( str.length === 0 ) { - return false; - } - - // Check if the string is a number - if ( ! isNaN( str ) ) { - return true; - } - - // Check if the string contains only numeric characters - if ( /^\d+$/.test( str ) ) { - return true; - } - - return false; -} - getEntityRecord.normalizeArgs = ( args ) => { let key = args && args[ 2 ]; 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..76006542feb35a --- /dev/null +++ b/packages/core-data/src/utils/is-numeric-id.js @@ -0,0 +1,10 @@ +/** + * Checks a given string to determine if it's a numeric ID. + * For example, '123' is a numeric ID, but '123abc' is not. + * + * @param {string} str the string to determine if it's a numeric ID. + * @return {boolean} true if the string is a numeric ID, false otherwise. + */ +export default function isNumericID( str = '' ) { + return /^\s*\d+\s*$/.test( str ); +} From 99419ce0d4f1d63dfa5411f3db1ad8082d3bb50e Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Tue, 4 Jul 2023 10:24:37 +0100 Subject: [PATCH 05/33] Test for util --- .../core-data/src/utils/test/is-numeric-id.js | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 packages/core-data/src/utils/test/is-numeric-id.js 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..6a14898f73f17d --- /dev/null +++ b/packages/core-data/src/utils/test/is-numeric-id.js @@ -0,0 +1,21 @@ +/** + * 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' ], + [ true, 123 ], + ] )( `should return %s for input "%s"`, ( expected, input ) => { + expect( isNumericID( input ) ).toBe( expected ); + } ); +} ); From 82352d7688d4241b020312434c489909988e40de Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Tue, 4 Jul 2023 10:44:14 +0100 Subject: [PATCH 06/33] Add tests for normalizeArgs --- packages/data/src/redux-store/test/index.js | 45 +++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/packages/data/src/redux-store/test/index.js b/packages/data/src/redux-store/test/index.js index daca128480daeb..4c65f4822036d2 100644 --- a/packages/data/src/redux-store/test/index.js +++ b/packages/data/src/redux-store/test/index.js @@ -286,3 +286,48 @@ describe( 'resolveSelect', () => { ] ); } ); } ); + +describe( 'normalizing args', () => { + it( 'should call the .normalizeArgs method on the resolver if it exists', async () => { + const registry = createRegistry(); + const resolver = () => {}; + + resolver.normalizeArgs = jest.fn( ( ...args ) => args ); + + registry.registerStore( 'store', { + reducer: () => {}, + selectors: { + getItems: () => 'items', + }, + resolvers: { + getItems: resolver, + }, + } ); + registry.select( 'store' ).getItems( 'foo', 'bar' ); + + expect( resolver.normalizeArgs ).toHaveBeenCalledWith( [ + 'foo', + 'bar', + ] ); + } ); + + it( 'should not call normalizeArgs if there are no arguments passed to the resolver', async () => { + const registry = createRegistry(); + const resolver = () => {}; + + resolver.normalizeArgs = jest.fn( ( ...args ) => args ); + + registry.registerStore( 'store', { + reducer: () => {}, + selectors: { + getItems: () => 'items', + }, + resolvers: { + getItems: resolver, + }, + } ); + registry.select( 'store' ).getItems(); + + expect( resolver.normalizeArgs ).not.toHaveBeenCalled(); + } ); +} ); From 49eea82116aac9c87ebb474fbbbc37e40f909ede Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Tue, 4 Jul 2023 10:44:31 +0100 Subject: [PATCH 07/33] Fix bug with normalizeArgs called even if there are no args --- packages/data/src/redux-store/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/data/src/redux-store/index.js b/packages/data/src/redux-store/index.js index d07e5ca048d584..0395609eabca44 100644 --- a/packages/data/src/redux-store/index.js +++ b/packages/data/src/redux-store/index.js @@ -604,7 +604,7 @@ function mapSelectorWithResolver( } const selectorResolver = ( ...args ) => { - if ( resolver.normalizeArgs ) { + if ( resolver.normalizeArgs && args?.length ) { args = resolver.normalizeArgs( args ); } fulfillSelector( args ); From f569a98e0b639155644d977d61b5af09ee5f218e Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Tue, 4 Jul 2023 13:55:28 +0100 Subject: [PATCH 08/33] Add direct test on `normalizeArgs` method of getEntityRecord --- packages/core-data/src/test/resolvers.js | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/packages/core-data/src/test/resolvers.js b/packages/core-data/src/test/resolvers.js index 544caad0c2dbc4..76fe1de40032b3 100644 --- a/packages/core-data/src/test/resolvers.js +++ b/packages/core-data/src/test/resolvers.js @@ -122,6 +122,30 @@ describe( 'getEntityRecord', () => { 1 ); } ); + + describe( 'normalizing Post ID passed as recordKey', () => { + it( 'normalizes any Post ID recordKey argument to a Number via `normalizeArgs` method', async () => { + const normalized = getEntityRecord.normalizeArgs( [ + '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.normalizeArgs( [ + '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', + ] ); + } ); + } ); } ); describe( 'getEntityRecords', () => { From cd31934e8637cb0218a7d38627b4b5898e4fda53 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Tue, 4 Jul 2023 15:42:08 +0100 Subject: [PATCH 09/33] Allow defining normalize method on selector and call for both selector and resolver --- packages/data/src/redux-store/index.js | 21 ++++++++++-- packages/data/src/redux-store/test/index.js | 36 ++++++++++++--------- 2 files changed, 40 insertions(+), 17 deletions(-) diff --git a/packages/data/src/redux-store/index.js b/packages/data/src/redux-store/index.js index 0395609eabca44..4ed82f50d816bc 100644 --- a/packages/data/src/redux-store/index.js +++ b/packages/data/src/redux-store/index.js @@ -235,11 +235,24 @@ export default function createReduxStore( key, options ) { selector.registry = registry; } const boundSelector = ( ...args ) => { + if ( + selector.normalizeArgs && + typeof selector.normalizeArgs === 'function' && + args?.length + ) { + args = selector.normalizeArgs( 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.normalizeArgs = selector.normalizeArgs; + const resolver = resolvers[ selectorName ]; + if ( ! resolver ) { boundSelector.hasResolver = false; return boundSelector; @@ -604,8 +617,12 @@ function mapSelectorWithResolver( } const selectorResolver = ( ...args ) => { - if ( resolver.normalizeArgs && args?.length ) { - args = resolver.normalizeArgs( args ); + if ( + selector.normalizeArgs && + typeof selector.normalizeArgs === 'function' && + args?.length + ) { + args = selector.normalizeArgs( args ); } fulfillSelector( args ); return selector( ...args ); diff --git a/packages/data/src/redux-store/test/index.js b/packages/data/src/redux-store/test/index.js index 4c65f4822036d2..9c06b6bff80c43 100644 --- a/packages/data/src/redux-store/test/index.js +++ b/packages/data/src/redux-store/test/index.js @@ -288,46 +288,52 @@ describe( 'resolveSelect', () => { } ); describe( 'normalizing args', () => { - it( 'should call the .normalizeArgs method on the resolver if it exists', async () => { + it( 'should call the normalizeArgs method of the selector for both the selector and the resolver', async () => { const registry = createRegistry(); - const resolver = () => {}; + const selector = () => {}; - resolver.normalizeArgs = jest.fn( ( ...args ) => args ); + const normalizingFunction = jest.fn( ( ...args ) => args ); + + selector.normalizeArgs = normalizingFunction; registry.registerStore( 'store', { reducer: () => {}, selectors: { - getItems: () => 'items', + getItems: selector, }, resolvers: { - getItems: resolver, + getItems: () => 'items', }, } ); registry.select( 'store' ).getItems( 'foo', 'bar' ); - expect( resolver.normalizeArgs ).toHaveBeenCalledWith( [ - 'foo', - 'bar', - ] ); + expect( normalizingFunction ).toHaveBeenCalledWith( [ 'foo', 'bar' ] ); + + // Needs to be call twice: + // 1. When the selector is called. + // 2. When the resolver is fullfilled. + expect( normalizingFunction ).toHaveBeenCalledTimes( 2 ); } ); - it( 'should not call normalizeArgs if there are no arguments passed to the resolver', async () => { + it( 'should not call the normalizeArgs method if there are no arguments passed to the selector (and thus the resolver)', async () => { const registry = createRegistry(); - const resolver = () => {}; + const selector = () => {}; - resolver.normalizeArgs = jest.fn( ( ...args ) => args ); + selector.normalizeArgs = jest.fn( ( ...args ) => args ); registry.registerStore( 'store', { reducer: () => {}, selectors: { - getItems: () => 'items', + getItems: selector, }, resolvers: { - getItems: resolver, + getItems: () => 'items', }, } ); + + // Called with no args so the normalizeArgs method should not be called. registry.select( 'store' ).getItems(); - expect( resolver.normalizeArgs ).not.toHaveBeenCalled(); + expect( selector.normalizeArgs ).not.toHaveBeenCalled(); } ); } ); From 775f32b4fcecc92497d3a4fb478b9e20904fb184 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Tue, 4 Jul 2023 15:43:46 +0100 Subject: [PATCH 10/33] Move normalization function to getEntityRecord selector --- packages/core-data/src/resolvers.js | 18 +----------------- packages/core-data/src/selectors.ts | 13 +++++++++++++ packages/core-data/src/test/resolvers.js | 24 ------------------------ packages/core-data/src/test/selectors.js | 23 +++++++++++++++++++++++ 4 files changed, 37 insertions(+), 41 deletions(-) diff --git a/packages/core-data/src/resolvers.js b/packages/core-data/src/resolvers.js index 4a26541b664497..07e9cd98cb5ec3 100644 --- a/packages/core-data/src/resolvers.js +++ b/packages/core-data/src/resolvers.js @@ -15,11 +15,7 @@ import apiFetch from '@wordpress/api-fetch'; */ import { STORE_NAME } from './name'; import { getOrLoadEntitiesConfig, DEFAULT_ENTITY_KEY } from './entities'; -import { - forwardResolver, - getNormalizedCommaSeparable, - isNumericID, -} from './utils'; +import { forwardResolver, getNormalizedCommaSeparable } from './utils'; import { getSyncProvider } from './sync'; /** @@ -176,18 +172,6 @@ export const getEntityRecord = } }; -getEntityRecord.normalizeArgs = ( args ) => { - let key = args && args[ 2 ]; - - // If key is numeric, assume it's an ID and coerce to number. - if ( key && typeof key === 'string' && isNumericID( key ) ) { - key = Number( key ); - } - - args[ 2 ] = key; - return args; -}; - /** * Requests an entity's record from the REST API. */ diff --git a/packages/core-data/src/selectors.ts b/packages/core-data/src/selectors.ts index f242f757d4becd..d038baa446d2b6 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'; @@ -365,6 +366,18 @@ export const getEntityRecord = createSelector( } ) as GetEntityRecord; +getEntityRecord.normalizeArgs = ( args ) => { + let key = args && args[ 2 ]; + + // If key is numeric, assume it's an ID and coerce to number. + if ( key && typeof key === 'string' && isNumericID( key ) ) { + key = Number( key ); + } + + args[ 2 ] = key; + return args; +}; + /** * 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/resolvers.js b/packages/core-data/src/test/resolvers.js index 76fe1de40032b3..544caad0c2dbc4 100644 --- a/packages/core-data/src/test/resolvers.js +++ b/packages/core-data/src/test/resolvers.js @@ -122,30 +122,6 @@ describe( 'getEntityRecord', () => { 1 ); } ); - - describe( 'normalizing Post ID passed as recordKey', () => { - it( 'normalizes any Post ID recordKey argument to a Number via `normalizeArgs` method', async () => { - const normalized = getEntityRecord.normalizeArgs( [ - '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.normalizeArgs( [ - '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', - ] ); - } ); - } ); } ); describe( 'getEntityRecords', () => { diff --git a/packages/core-data/src/test/selectors.js b/packages/core-data/src/test/selectors.js index 61c0621b6e5e49..a33257393312c0 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 `normalizeArgs` method', async () => { + const normalized = getEntityRecord.normalizeArgs( [ + '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.normalizeArgs( [ + '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: { From 240075b264c55255898206dbb87da066bc7c484c Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Tue, 4 Jul 2023 15:45:46 +0100 Subject: [PATCH 11/33] Move normalization functino to getEntityRecord selector --- packages/core-data/src/selectors.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/core-data/src/selectors.ts b/packages/core-data/src/selectors.ts index d038baa446d2b6..fe9dfdca1d8679 100644 --- a/packages/core-data/src/selectors.ts +++ b/packages/core-data/src/selectors.ts @@ -271,6 +271,7 @@ export function getEntityConfig( * See https://github.com/WordPress/gutenberg/pull/41578 for more details. */ export interface GetEntityRecord { + normalizeArgs: ( args: any ) => any; < EntityRecord extends | ET.EntityRecord< any > From f9a3fb74cde0d86af965e762d7170d074b009644 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Tue, 4 Jul 2023 16:29:41 +0100 Subject: [PATCH 12/33] Add rough outline of documentation --- packages/data/README.md | 46 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/packages/data/README.md b/packages/data/README.md index 96115e734266ef..d0a73952f96821 100644 --- a/packages/data/README.md +++ b/packages/data/README.md @@ -1127,6 +1127,52 @@ _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. This is necessary because selectors are pre-bound to resolvers before they applied. Moreover, metadata selectors/resolvers are also created using the supplied arguments. + +In these circumstances selectors may optionally define a custom property ("method") called `normalizeArgs` which should be a function thats returns the normalized form of the arguments. + +For example, if the 3rd argument of the selector is supposed to be an `Number` but it is called with a `String` then normalizeArgs can be defined to coerce the argument type: + +```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 {}; +} + +// Define normalization function. +getItemsSelector.normalizeArgs = ( args ) { + // "id" argument at the 2nd index + if (args[2] && typeof args[2] === 'string' ) { + args[2] === Number(args[2]); + } + + return args; +} + +registry.registerStore( 'store', { + // ... + selectors: { + getItems: getItemsSelector, + }, + resolvers: { + getItems: getItemsResolver, + }, +} ); + +// '54' is guaranteed to be coerced to a number/ +registry.select( 'store' ).getItems( 'foo', 'bar', '54' ); +``` + +This is particularly important to ensure that resolvers are cached correctly. Resolvers are intended to be invoked once per selector call and are then marked as resolved. However, resolvers are keyed in a cache by their arguments. Therefore ensuring type consistency between these arguments is important to avoid cache misses. + ## Going further - [What is WordPress Data?](https://unfoldingneurons.com/2020/what-is-wordpress-data/) From 728b5f7f33320d368163ee241a8e94e7b51badde Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Tue, 4 Jul 2023 16:47:40 +0100 Subject: [PATCH 13/33] Add test to ensure normalization is applied to selector even without matching resolver --- packages/data/README.md | 1 - packages/data/src/redux-store/test/index.js | 21 +++++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/packages/data/README.md b/packages/data/README.md index d0a73952f96821..af049defed12de 100644 --- a/packages/data/README.md +++ b/packages/data/README.md @@ -1136,7 +1136,6 @@ In these circumstances selectors may optionally define a custom property ("metho For example, if the 3rd argument of the selector is supposed to be an `Number` but it is called with a `String` then normalizeArgs can be defined to coerce the argument type: ```js - const getItemsSelector = ( name, type, id ) => { // here 'id' is now guaranteed to be a number. return state.items[name][type][id] || null; diff --git a/packages/data/src/redux-store/test/index.js b/packages/data/src/redux-store/test/index.js index 9c06b6bff80c43..0749752ebe9caf 100644 --- a/packages/data/src/redux-store/test/index.js +++ b/packages/data/src/redux-store/test/index.js @@ -336,4 +336,25 @@ describe( 'normalizing args', () => { expect( selector.normalizeArgs ).not.toHaveBeenCalled(); } ); + + it( 'should call the normalizeArgs method on the selectors without resolvers', async () => { + const registry = createRegistry(); + const selector = () => {}; + + selector.normalizeArgs = jest.fn( ( ...args ) => args ); + + registry.registerStore( 'store', { + reducer: () => {}, + selectors: { + getItems: selector, + }, + } ); + + registry.select( 'store' ).getItems( 'foo', 'bar' ); + + expect( selector.normalizeArgs ).toHaveBeenCalledWith( [ + 'foo', + 'bar', + ] ); + } ); } ); From 3c435493e92625010dc9013f5b80f348917bc50d Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 5 Jul 2023 13:22:02 +0100 Subject: [PATCH 14/33] Improve documentation to better explain concept of normalizeArgs --- packages/data/README.md | 60 ++++++++++++++++++++++++++++++++--------- 1 file changed, 48 insertions(+), 12 deletions(-) diff --git a/packages/data/README.md b/packages/data/README.md index af049defed12de..b1216d95bf0e96 100644 --- a/packages/data/README.md +++ b/packages/data/README.md @@ -1129,24 +1129,41 @@ _Returns_ ### Normalizing Selector Arguments -In specific circumstances it may be necessary to normalize the arguments passed to a given call of a selector. This is necessary because selectors are pre-bound to resolvers before they applied. Moreover, metadata selectors/resolvers are also created using the supplied arguments. +In specific circumstances it may be necessary to normalize the arguments passed to a given _call_ of a selector/resolver pairing. -In these circumstances selectors may optionally define a custom property ("method") called `normalizeArgs` which should be a function thats returns the normalized form of the arguments. +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, if the 3rd argument of the selector is supposed to be an `Number` but it is called with a `String` then normalizeArgs can be defined to coerce the argument type: +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 `normalizeArgs` property to guarantee consistency. + +#### Example + +For example, if the 3rd argument of the following selector is intended to be a `Number`: ```js const getItemsSelector = ( name, type, id ) => { // here 'id' is now guaranteed to be a number. - return state.items[name][type][id] || null; -} + return state.items[ name ][ type ][ id ] || null; +}; +``` -const getItemsResolver = ( name, type, id ) => { - // 'id' is also guaranteed to be a number in the resolver. - return {}; -} +However, it is possible that the `id` parameter will be passed as a `String`. In this case, the `normalizeArgs` method (property) can be defined on the _selector_ to coerce the arguments to the desired type: -// Define normalization function. +```js +// Define normalization method. getItemsSelector.normalizeArgs = ( args ) { // "id" argument at the 2nd index if (args[2] && typeof args[2] === 'string' ) { @@ -1155,6 +1172,20 @@ getItemsSelector.normalizeArgs = ( args ) { return args; } +``` + +With this in place the following code will behave consistency: + +```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', { // ... @@ -1166,11 +1197,16 @@ registry.registerStore( 'store', { }, } ); -// '54' is guaranteed to be coerced to a number/ +// 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 `normalizeArgs` method. registry.select( 'store' ).getItems( 'foo', 'bar', '54' ); ``` -This is particularly important to ensure that resolvers are cached correctly. Resolvers are intended to be invoked once per selector call and are then marked as resolved. However, resolvers are keyed in a cache by their arguments. Therefore ensuring type consistency between these arguments is important to avoid cache misses. +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). ## Going further From 4c5aa4349bec0da567496290eb7ac0ed157126d9 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 5 Jul 2023 13:23:01 +0100 Subject: [PATCH 15/33] Correct grammar --- packages/data/src/redux-store/test/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/data/src/redux-store/test/index.js b/packages/data/src/redux-store/test/index.js index 0749752ebe9caf..e03d9755e5935b 100644 --- a/packages/data/src/redux-store/test/index.js +++ b/packages/data/src/redux-store/test/index.js @@ -309,7 +309,7 @@ describe( 'normalizing args', () => { expect( normalizingFunction ).toHaveBeenCalledWith( [ 'foo', 'bar' ] ); - // Needs to be call twice: + // Needs to be called twice: // 1. When the selector is called. // 2. When the resolver is fullfilled. expect( normalizingFunction ).toHaveBeenCalledTimes( 2 ); From 959fe56eabb60c0c0a1af2b284c59a63ebe83790 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 5 Jul 2023 13:28:54 +0100 Subject: [PATCH 16/33] Apply types optimisation from code review See https://github.com/WordPress/gutenberg/pull/52120#discussion_r1253011269 --- packages/core-data/src/selectors.ts | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/packages/core-data/src/selectors.ts b/packages/core-data/src/selectors.ts index fe9dfdca1d8679..607261b12e5fc0 100644 --- a/packages/core-data/src/selectors.ts +++ b/packages/core-data/src/selectors.ts @@ -96,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 @@ -271,17 +278,16 @@ export function getEntityConfig( * See https://github.com/WordPress/gutenberg/pull/41578 for more details. */ export interface GetEntityRecord { - normalizeArgs: ( args: any ) => any; < EntityRecord extends | ET.EntityRecord< any > | Partial< ET.EntityRecord< any > >, >( state: State, - kind: string, - name: string, - key: EntityRecordKey, - query?: GetRecordsHttpQuery + kind: EntityRecordArgs[ 0 ], + name: EntityRecordArgs[ 1 ], + key: EntityRecordArgs[ 2 ], + query?: EntityRecordArgs[ 3 ] ): EntityRecord | undefined; CurriedSignature: < @@ -289,11 +295,12 @@ export interface GetEntityRecord { | ET.EntityRecord< any > | Partial< ET.EntityRecord< any > >, >( - kind: string, - name: string, - key: EntityRecordKey, - query?: GetRecordsHttpQuery + kind: EntityRecordArgs[ 0 ], + name: EntityRecordArgs[ 1 ], + key: EntityRecordArgs[ 2 ], + query?: EntityRecordArgs[ 3 ] ) => EntityRecord | undefined; + normalizeArgs?: ( args: EntityRecordArgs ) => EntityRecordArgs; } /** From 7aa49edd9d5a6c4387fbbb9e31eb0adf7a880fa7 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 5 Jul 2023 13:40:17 +0100 Subject: [PATCH 17/33] Extract conditionals to helper function As per code review --- packages/data/src/redux-store/index.js | 35 +++++++++++++++----------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/packages/data/src/redux-store/index.js b/packages/data/src/redux-store/index.js index 4ed82f50d816bc..065a8fbe711e36 100644 --- a/packages/data/src/redux-store/index.js +++ b/packages/data/src/redux-store/index.js @@ -235,13 +235,7 @@ export default function createReduxStore( key, options ) { selector.registry = registry; } const boundSelector = ( ...args ) => { - if ( - selector.normalizeArgs && - typeof selector.normalizeArgs === 'function' && - args?.length - ) { - args = selector.normalizeArgs( args ); - } + args = normalize( selector, args ); const state = store.__unstableOriginalGetState(); return selector( state.root, ...args ); }; @@ -617,16 +611,29 @@ function mapSelectorWithResolver( } const selectorResolver = ( ...args ) => { - if ( - selector.normalizeArgs && - typeof selector.normalizeArgs === 'function' && - args?.length - ) { - args = selector.normalizeArgs( 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.normalizeArgs && + typeof selector.normalizeArgs === 'function' + ) { + return selector.normalizeArgs( args ); + } + return args; +} From 39652847706b03d9c76c0c2b062fa73262464df1 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 5 Jul 2023 13:44:17 +0100 Subject: [PATCH 18/33] Document getEntityRecord normalization function --- packages/core-data/src/selectors.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/core-data/src/selectors.ts b/packages/core-data/src/selectors.ts index 607261b12e5fc0..fae644b72a424f 100644 --- a/packages/core-data/src/selectors.ts +++ b/packages/core-data/src/selectors.ts @@ -374,6 +374,12 @@ export const getEntityRecord = createSelector( } ) as GetEntityRecord; +/** + * Normalizes `recordKey`s that look like numeric IDs to numbers. + * + * @param args EntityRecordArgs the selector arguments. + * @return the normalized selector arguments. + */ getEntityRecord.normalizeArgs = ( args ) => { let key = args && args[ 2 ]; From 54d76c7a29cca3f55cb76257eaec607732d55b97 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 5 Jul 2023 13:46:46 +0100 Subject: [PATCH 19/33] Add type defs to normalization function --- packages/core-data/src/selectors.ts | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/packages/core-data/src/selectors.ts b/packages/core-data/src/selectors.ts index fae644b72a424f..5a2ef975808673 100644 --- a/packages/core-data/src/selectors.ts +++ b/packages/core-data/src/selectors.ts @@ -378,17 +378,23 @@ export const getEntityRecord = createSelector( * Normalizes `recordKey`s that look like numeric IDs to numbers. * * @param args EntityRecordArgs the selector arguments. - * @return the normalized selector arguments. + * @return EntityRecordArgs the normalized arguments. */ -getEntityRecord.normalizeArgs = ( args ) => { - let key = args && args[ 2 ]; - - // If key is numeric, assume it's an ID and coerce to number. - if ( key && typeof key === 'string' && isNumericID( key ) ) { - key = Number( key ); +getEntityRecord.normalizeArgs = ( + args: EntityRecordArgs +): EntityRecordArgs => { + let recordKey = args && args[ 2 ]; + + // If recordKey looks to be a numeric ID then coerce to number. + if ( + recordKey && + typeof recordKey === 'string' && + isNumericID( recordKey ) + ) { + recordKey = Number( recordKey ); } - args[ 2 ] = key; + args[ 2 ] = recordKey; return args; }; From 0d5856ee30d0377245a6b7e12c53fde41ecdc18b Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 5 Jul 2023 13:52:48 +0100 Subject: [PATCH 20/33] Fix nits in README --- packages/data/README.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/data/README.md b/packages/data/README.md index b1216d95bf0e96..060b7cca481370 100644 --- a/packages/data/README.md +++ b/packages/data/README.md @@ -1147,11 +1147,11 @@ getSomeDataById( 123 ); getSomeDataById( '123' ); ``` -This is an opportunity to utilize the `normalizeArgs` property to guarantee consistency. +This is an opportunity to utilize the `normalizeArgs` property to guarantee consistency by protecting callers from passing incorrect types. #### Example -For example, if the 3rd argument of the following selector is intended to be a `Number`: +The _3rd_ argument of the following selector is intended to be a `Number`: ```js const getItemsSelector = ( name, type, id ) => { @@ -1160,7 +1160,7 @@ const getItemsSelector = ( name, type, id ) => { }; ``` -However, it is possible that the `id` parameter will be passed as a `String`. In this case, the `normalizeArgs` method (property) can be defined on the _selector_ to coerce the arguments to the desired type: +However, it is possible that the `id` parameter will be passed as a `String`. In this case, the `normalizeArgs` 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. @@ -1174,7 +1174,7 @@ getItemsSelector.normalizeArgs = ( args ) { } ``` -With this in place the following code will behave consistency: +With this in place the following code will behave consistently: ```js const getItemsSelector = ( name, type, id ) => { From 7683ab33022d16bf33d6d1f92f1e55cab9d6ca4e Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 5 Jul 2023 13:53:42 +0100 Subject: [PATCH 21/33] Remove new line --- packages/data/src/redux-store/index.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/data/src/redux-store/index.js b/packages/data/src/redux-store/index.js index 065a8fbe711e36..415c583f117dc0 100644 --- a/packages/data/src/redux-store/index.js +++ b/packages/data/src/redux-store/index.js @@ -612,7 +612,6 @@ function mapSelectorWithResolver( const selectorResolver = ( ...args ) => { args = normalize( selector, args ); - fulfillSelector( args ); return selector( ...args ); }; From 7b4cdfbbbbfbd35616c259b51320aea09c33e6d0 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 5 Jul 2023 13:55:18 +0100 Subject: [PATCH 22/33] Add test for arguments length before invoking normalization function --- packages/data/src/redux-store/index.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/data/src/redux-store/index.js b/packages/data/src/redux-store/index.js index 415c583f117dc0..385267e9e53ebf 100644 --- a/packages/data/src/redux-store/index.js +++ b/packages/data/src/redux-store/index.js @@ -630,7 +630,8 @@ function mapSelectorWithResolver( function normalize( selector, args ) { if ( selector.normalizeArgs && - typeof selector.normalizeArgs === 'function' + typeof selector.normalizeArgs === 'function' && + args?.length ) { return selector.normalizeArgs( args ); } From 6702752c133ec07fe9cd5851cb521be4f819c247 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 5 Jul 2023 13:58:06 +0100 Subject: [PATCH 23/33] Remove unwanted comment --- packages/data/README.md | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/data/README.md b/packages/data/README.md index 060b7cca481370..7fb356e37b7f8f 100644 --- a/packages/data/README.md +++ b/packages/data/README.md @@ -1155,7 +1155,6 @@ The _3rd_ argument of the following selector is intended to be a `Number`: ```js const getItemsSelector = ( name, type, id ) => { - // here 'id' is now guaranteed to be a number. return state.items[ name ][ type ][ id ] || null; }; ``` From be322a4a9cfdf31d8ae1b1bc722a4fd0a9ab53b0 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 5 Jul 2023 16:27:46 +0100 Subject: [PATCH 24/33] Addition to docs as per code review https://github.com/WordPress/gutenberg/pull/52120#discussion_r1253117733 --- packages/data/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/data/README.md b/packages/data/README.md index 7fb356e37b7f8f..7fb78e7e6f4ad2 100644 --- a/packages/data/README.md +++ b/packages/data/README.md @@ -1205,7 +1205,7 @@ registry.select( 'store' ).getItems( 'foo', 'bar', 54 ); 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). +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 From c178aa5be1715d29425af2d18bf5d66075402f82 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Thu, 6 Jul 2023 10:19:29 +0100 Subject: [PATCH 25/33] Fix bug with metadata selector args not being normalized --- packages/data/src/redux-store/index.js | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/packages/data/src/redux-store/index.js b/packages/data/src/redux-store/index.js index 385267e9e53ebf..9507b8cb5854a1 100644 --- a/packages/data/src/redux-store/index.js +++ b/packages/data/src/redux-store/index.js @@ -261,10 +261,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; From f375fd72a925154c5fac22d3666de26762f4f62a Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Thu, 6 Jul 2023 10:40:32 +0100 Subject: [PATCH 26/33] Add tests for normalization to metadata selector tests --- .../redux-store/metadata/test/selectors.js | 40 ++++++++++++++++++- 1 file changed, 38 insertions(+), 2 deletions(-) diff --git a/packages/data/src/redux-store/metadata/test/selectors.js b/packages/data/src/redux-store/metadata/test/selectors.js index 11eba0301e8c55..f391640425c95d 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.normalizeArgs = 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.normalizeArgs = undefined; + } ); +} ); From d4899d206722178072cbea2e0f9711fb95bea0d4 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Fri, 7 Jul 2023 13:26:12 +0100 Subject: [PATCH 27/33] Add test case for decimals --- packages/core-data/src/utils/test/is-numeric-id.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/core-data/src/utils/test/is-numeric-id.js b/packages/core-data/src/utils/test/is-numeric-id.js index 6a14898f73f17d..ac879887742561 100644 --- a/packages/core-data/src/utils/test/is-numeric-id.js +++ b/packages/core-data/src/utils/test/is-numeric-id.js @@ -14,6 +14,7 @@ describe( 'isNumericID', () => { [ 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 ); From 9ad97443a3b9319fa50922cafc81f758536ddbec Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 12 Jul 2023 09:09:16 +0100 Subject: [PATCH 28/33] =?UTF-8?q?Prefix=20with=20=5F=5Funstable=20to=20den?= =?UTF-8?q?ote=20non=20=E2=80=9Csettled=E2=80=9D=20approach=20amongst=20co?= =?UTF-8?q?ntributors?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/core-data/src/selectors.ts | 4 ++-- packages/core-data/src/test/selectors.js | 6 +++--- packages/data/README.md | 8 ++++---- .../src/redux-store/metadata/test/selectors.js | 4 ++-- packages/data/src/redux-store/test/index.js | 18 +++++++++--------- 5 files changed, 20 insertions(+), 20 deletions(-) diff --git a/packages/core-data/src/selectors.ts b/packages/core-data/src/selectors.ts index 5a2ef975808673..a70a56de862d4c 100644 --- a/packages/core-data/src/selectors.ts +++ b/packages/core-data/src/selectors.ts @@ -300,7 +300,7 @@ export interface GetEntityRecord { key: EntityRecordArgs[ 2 ], query?: EntityRecordArgs[ 3 ] ) => EntityRecord | undefined; - normalizeArgs?: ( args: EntityRecordArgs ) => EntityRecordArgs; + __unstableNormalizeArgs?: ( args: EntityRecordArgs ) => EntityRecordArgs; } /** @@ -380,7 +380,7 @@ export const getEntityRecord = createSelector( * @param args EntityRecordArgs the selector arguments. * @return EntityRecordArgs the normalized arguments. */ -getEntityRecord.normalizeArgs = ( +getEntityRecord.__unstableNormalizeArgs = ( args: EntityRecordArgs ): EntityRecordArgs => { let recordKey = args && args[ 2 ]; diff --git a/packages/core-data/src/test/selectors.js b/packages/core-data/src/test/selectors.js index a33257393312c0..b7c583098c4a5c 100644 --- a/packages/core-data/src/test/selectors.js +++ b/packages/core-data/src/test/selectors.js @@ -29,8 +29,8 @@ describe.each( [ [ __experimentalGetEntityRecordNoResolver ], ] )( '%p', ( selector ) => { describe( 'normalizing Post ID passed as recordKey', () => { - it( 'normalizes any Post ID recordKey argument to a Number via `normalizeArgs` method', async () => { - const normalized = getEntityRecord.normalizeArgs( [ + it( 'normalizes any Post ID recordKey argument to a Number via `__unstableNormalizeArgs` method', async () => { + const normalized = getEntityRecord.__unstableNormalizeArgs( [ 'postType', 'some_post', '123', @@ -39,7 +39,7 @@ describe.each( [ } ); it( 'does not normalize recordKey argument unless it is a Post ID', async () => { - const normalized = getEntityRecord.normalizeArgs( [ + const normalized = getEntityRecord.__unstableNormalizeArgs( [ 'postType', 'some_post', 'i-am-a-slug-with-a-number-123', diff --git a/packages/data/README.md b/packages/data/README.md index 7fb78e7e6f4ad2..7a12a913e661e2 100644 --- a/packages/data/README.md +++ b/packages/data/README.md @@ -1147,7 +1147,7 @@ getSomeDataById( 123 ); getSomeDataById( '123' ); ``` -This is an opportunity to utilize the `normalizeArgs` property to guarantee consistency by protecting callers from passing incorrect types. +This is an opportunity to utilize the `__unstableNormalizeArgs` property to guarantee consistency by protecting callers from passing incorrect types. #### Example @@ -1159,11 +1159,11 @@ const getItemsSelector = ( name, type, id ) => { }; ``` -However, it is possible that the `id` parameter will be passed as a `String`. In this case, the `normalizeArgs` method (property) can be defined on the _selector_ to coerce the arguments to the desired type even if they are provided "incorrectly": +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.normalizeArgs = ( args ) { +getItemsSelector.__unstableNormalizeArgs = ( args ) { // "id" argument at the 2nd index if (args[2] && typeof args[2] === 'string' ) { args[2] === Number(args[2]); @@ -1201,7 +1201,7 @@ 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 `normalizeArgs` method. +// coerced to a number by the `__unstableNormalizeArgs` method. registry.select( 'store' ).getItems( 'foo', 'bar', '54' ); ``` diff --git a/packages/data/src/redux-store/metadata/test/selectors.js b/packages/data/src/redux-store/metadata/test/selectors.js index f391640425c95d..faf9da3a169853 100644 --- a/packages/data/src/redux-store/metadata/test/selectors.js +++ b/packages/data/src/redux-store/metadata/test/selectors.js @@ -457,7 +457,7 @@ describe( 'Selector arguments normalization', () => { const normalizationFunction = jest.fn( ( args ) => { return args.map( Number ); } ); - getFooSelector.normalizeArgs = normalizationFunction; + getFooSelector.__unstableNormalizeArgs = normalizationFunction; registry.dispatch( 'testStore' ).startResolution( 'getFoo', [ 123 ] ); const { getIsResolving, hasStartedResolution, hasFinishedResolution } = @@ -476,6 +476,6 @@ describe( 'Selector arguments normalization', () => { expect( hasFinishedResolution( 'getFoo', [ '123' ] ) ).toBe( true ); expect( normalizationFunction ).toHaveBeenCalledWith( [ '123' ] ); - getFooSelector.normalizeArgs = undefined; + getFooSelector.__unstableNormalizeArgs = undefined; } ); } ); diff --git a/packages/data/src/redux-store/test/index.js b/packages/data/src/redux-store/test/index.js index e03d9755e5935b..1251051c4c9d9a 100644 --- a/packages/data/src/redux-store/test/index.js +++ b/packages/data/src/redux-store/test/index.js @@ -288,13 +288,13 @@ describe( 'resolveSelect', () => { } ); describe( 'normalizing args', () => { - it( 'should call the normalizeArgs method of the selector for both the selector and the resolver', async () => { + 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.normalizeArgs = normalizingFunction; + selector.__unstableNormalizeArgs = normalizingFunction; registry.registerStore( 'store', { reducer: () => {}, @@ -315,11 +315,11 @@ describe( 'normalizing args', () => { expect( normalizingFunction ).toHaveBeenCalledTimes( 2 ); } ); - it( 'should not call the normalizeArgs method if there are no arguments passed to the selector (and thus the resolver)', async () => { + 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.normalizeArgs = jest.fn( ( ...args ) => args ); + selector.__unstableNormalizeArgs = jest.fn( ( ...args ) => args ); registry.registerStore( 'store', { reducer: () => {}, @@ -331,17 +331,17 @@ describe( 'normalizing args', () => { }, } ); - // Called with no args so the normalizeArgs method should not be called. + // Called with no args so the __unstableNormalizeArgs method should not be called. registry.select( 'store' ).getItems(); - expect( selector.normalizeArgs ).not.toHaveBeenCalled(); + expect( selector.__unstableNormalizeArgs ).not.toHaveBeenCalled(); } ); - it( 'should call the normalizeArgs method on the selectors without resolvers', async () => { + it( 'should call the __unstableNormalizeArgs method on the selectors without resolvers', async () => { const registry = createRegistry(); const selector = () => {}; - selector.normalizeArgs = jest.fn( ( ...args ) => args ); + selector.__unstableNormalizeArgs = jest.fn( ( ...args ) => args ); registry.registerStore( 'store', { reducer: () => {}, @@ -352,7 +352,7 @@ describe( 'normalizing args', () => { registry.select( 'store' ).getItems( 'foo', 'bar' ); - expect( selector.normalizeArgs ).toHaveBeenCalledWith( [ + expect( selector.__unstableNormalizeArgs ).toHaveBeenCalledWith( [ 'foo', 'bar', ] ); From 4598fa095ae9afb80a1a3816c4aa0bad58c8c978 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 12 Jul 2023 09:14:06 +0100 Subject: [PATCH 29/33] Remove check for args and conditionally access args index Addresses https://github.com/WordPress/gutenberg/pull/52120#discussion_r1254282367 --- packages/core-data/src/selectors.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core-data/src/selectors.ts b/packages/core-data/src/selectors.ts index a70a56de862d4c..4c0b15c3e30ab6 100644 --- a/packages/core-data/src/selectors.ts +++ b/packages/core-data/src/selectors.ts @@ -383,7 +383,7 @@ export const getEntityRecord = createSelector( getEntityRecord.__unstableNormalizeArgs = ( args: EntityRecordArgs ): EntityRecordArgs => { - let recordKey = args && args[ 2 ]; + let recordKey = args?.[ 2 ]; // If recordKey looks to be a numeric ID then coerce to number. if ( @@ -392,9 +392,9 @@ getEntityRecord.__unstableNormalizeArgs = ( isNumericID( recordKey ) ) { recordKey = Number( recordKey ); + args[ 2 ] = recordKey; } - args[ 2 ] = recordKey; return args; }; From 3a1dbb5af0a4a1fe24a9b93e63a663a7f9f1e19c Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 12 Jul 2023 09:20:43 +0100 Subject: [PATCH 30/33] Simplify coercing to number --- packages/core-data/src/selectors.ts | 11 ++--------- packages/core-data/src/utils/is-numeric-id.js | 8 ++++---- 2 files changed, 6 insertions(+), 13 deletions(-) diff --git a/packages/core-data/src/selectors.ts b/packages/core-data/src/selectors.ts index 4c0b15c3e30ab6..5c0a126216de6b 100644 --- a/packages/core-data/src/selectors.ts +++ b/packages/core-data/src/selectors.ts @@ -383,17 +383,10 @@ export const getEntityRecord = createSelector( getEntityRecord.__unstableNormalizeArgs = ( args: EntityRecordArgs ): EntityRecordArgs => { - let recordKey = args?.[ 2 ]; + const recordKey = args?.[ 2 ]; // If recordKey looks to be a numeric ID then coerce to number. - if ( - recordKey && - typeof recordKey === 'string' && - isNumericID( recordKey ) - ) { - recordKey = Number( recordKey ); - args[ 2 ] = recordKey; - } + args[ 2 ] = isNumericID( recordKey ) ? Number( recordKey ) : recordKey; return args; }; diff --git a/packages/core-data/src/utils/is-numeric-id.js b/packages/core-data/src/utils/is-numeric-id.js index 76006542feb35a..1bc05575d0aeda 100644 --- a/packages/core-data/src/utils/is-numeric-id.js +++ b/packages/core-data/src/utils/is-numeric-id.js @@ -1,10 +1,10 @@ /** - * Checks a given string to determine if it's a numeric ID. + * Checks argument to determine if it's a numeric ID. * For example, '123' is a numeric ID, but '123abc' is not. * - * @param {string} str the string to determine if it's a numeric ID. + * @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( str = '' ) { - return /^\s*\d+\s*$/.test( str ); +export default function isNumericID( id ) { + return /^\s*\d+\s*$/.test( id ); } From 2c5dc7acef8b2ca44c57f6138d3649edcd7dc158 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 12 Jul 2023 09:22:33 +0100 Subject: [PATCH 31/33] Revert confusing DRY typescript --- packages/core-data/src/selectors.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/core-data/src/selectors.ts b/packages/core-data/src/selectors.ts index 5c0a126216de6b..e260d1d512c736 100644 --- a/packages/core-data/src/selectors.ts +++ b/packages/core-data/src/selectors.ts @@ -284,10 +284,10 @@ export interface GetEntityRecord { | Partial< ET.EntityRecord< any > >, >( state: State, - kind: EntityRecordArgs[ 0 ], - name: EntityRecordArgs[ 1 ], - key: EntityRecordArgs[ 2 ], - query?: EntityRecordArgs[ 3 ] + kind: string, + name: string, + key: EntityRecordKey, + query?: GetRecordsHttpQuery ): EntityRecord | undefined; CurriedSignature: < @@ -295,10 +295,10 @@ export interface GetEntityRecord { | ET.EntityRecord< any > | Partial< ET.EntityRecord< any > >, >( - kind: EntityRecordArgs[ 0 ], - name: EntityRecordArgs[ 1 ], - key: EntityRecordArgs[ 2 ], - query?: EntityRecordArgs[ 3 ] + kind: string, + name: string, + key: EntityRecordKey, + query?: GetRecordsHttpQuery ) => EntityRecord | undefined; __unstableNormalizeArgs?: ( args: EntityRecordArgs ) => EntityRecordArgs; } From b0b5b450af37d729cc9c38a1961468615e376cd9 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 12 Jul 2023 13:06:15 +0100 Subject: [PATCH 32/33] Fix renaming selector to `unstable` --- packages/data/src/redux-store/index.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/packages/data/src/redux-store/index.js b/packages/data/src/redux-store/index.js index 9507b8cb5854a1..e8394087215428 100644 --- a/packages/data/src/redux-store/index.js +++ b/packages/data/src/redux-store/index.js @@ -243,7 +243,8 @@ export default function createReduxStore( key, options ) { // Expose normalization method on the bound selector // in order that it can be called when fullfilling // the resolver. - boundSelector.normalizeArgs = selector.normalizeArgs; + boundSelector.__unstableNormalizeArgs = + selector.__unstableNormalizeArgs; const resolver = resolvers[ selectorName ]; @@ -643,11 +644,11 @@ function mapSelectorWithResolver( */ function normalize( selector, args ) { if ( - selector.normalizeArgs && - typeof selector.normalizeArgs === 'function' && + selector.__unstableNormalizeArgs && + typeof selector.__unstableNormalizeArgs === 'function' && args?.length ) { - return selector.normalizeArgs( args ); + return selector.__unstableNormalizeArgs( args ); } return args; } From 5d2c7b3ae01b0ae41c3f64a75a1f9154341260a7 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 12 Jul 2023 13:10:54 +0100 Subject: [PATCH 33/33] Copy to new args Addresses https://github.com/WordPress/gutenberg/pull/52120/files#r1260851646 --- packages/core-data/src/selectors.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/core-data/src/selectors.ts b/packages/core-data/src/selectors.ts index e260d1d512c736..b6b36fad2ee934 100644 --- a/packages/core-data/src/selectors.ts +++ b/packages/core-data/src/selectors.ts @@ -383,12 +383,13 @@ export const getEntityRecord = createSelector( getEntityRecord.__unstableNormalizeArgs = ( args: EntityRecordArgs ): EntityRecordArgs => { - const recordKey = args?.[ 2 ]; + const newArgs = [ ...args ] as EntityRecordArgs; + const recordKey = newArgs?.[ 2 ]; // If recordKey looks to be a numeric ID then coerce to number. - args[ 2 ] = isNumericID( recordKey ) ? Number( recordKey ) : recordKey; + newArgs[ 2 ] = isNumericID( recordKey ) ? Number( recordKey ) : recordKey; - return args; + return newArgs; }; /**