diff --git a/packages/block-editor/src/store/actions.js b/packages/block-editor/src/store/actions.js index 170c5192f3bee..d20c6dfa508c8 100644 --- a/packages/block-editor/src/store/actions.js +++ b/packages/block-editor/src/store/actions.js @@ -373,11 +373,11 @@ export const replaceBlocks = type: 'REPLACE_BLOCKS', clientIds, blocks, - time: Date.now(), indexToSelect, initialPosition, meta, } ); + dispatch.updateInsertUsage( blocks ); // To avoid a focus loss when removing the last block, assure there is // always a default block if the last of the blocks have been removed. dispatch.ensureDefaultBlock(); @@ -577,11 +577,11 @@ export const insertBlocks = blocks: allowedBlocks, index, rootClientId, - time: Date.now(), updateSelection, initialPosition: updateSelection ? initialPosition : null, meta, } ); + dispatch.updateInsertUsage( allowedBlocks ); } }; @@ -1394,7 +1394,6 @@ export function replaceInnerBlocks( blocks, updateSelection, initialPosition: updateSelection ? initialPosition : null, - time: Date.now(), }; } diff --git a/packages/block-editor/src/store/defaults.js b/packages/block-editor/src/store/defaults.js index 54877eafc3690..78fb35b112ef0 100644 --- a/packages/block-editor/src/store/defaults.js +++ b/packages/block-editor/src/store/defaults.js @@ -3,10 +3,6 @@ */ import { __, _x } from '@wordpress/i18n'; -export const PREFERENCES_DEFAULTS = { - insertUsage: {}, -}; - /** * The default editor settings * diff --git a/packages/block-editor/src/store/defaults.native.js b/packages/block-editor/src/store/defaults.native.js index e951c39b3d1a4..44ee33c36e8e9 100644 --- a/packages/block-editor/src/store/defaults.native.js +++ b/packages/block-editor/src/store/defaults.native.js @@ -1,10 +1,7 @@ /** * Internal dependencies */ -import { - PREFERENCES_DEFAULTS, - SETTINGS_DEFAULTS as SETTINGS, -} from './defaults.js'; +import { SETTINGS_DEFAULTS as SETTINGS } from './defaults.js'; const SETTINGS_DEFAULTS = { ...SETTINGS, @@ -20,4 +17,4 @@ const SETTINGS_DEFAULTS = { }, }; -export { PREFERENCES_DEFAULTS, SETTINGS_DEFAULTS }; +export { SETTINGS_DEFAULTS }; diff --git a/packages/block-editor/src/store/index.js b/packages/block-editor/src/store/index.js index 0bcc00cb5f6ae..181139aad69e6 100644 --- a/packages/block-editor/src/store/index.js +++ b/packages/block-editor/src/store/index.js @@ -1,7 +1,7 @@ /** * WordPress dependencies */ -import { createReduxStore, registerStore } from '@wordpress/data'; +import { createReduxStore, register } from '@wordpress/data'; /** * Internal dependencies @@ -32,24 +32,8 @@ export const storeConfig = { */ export const store = createReduxStore( STORE_NAME, { ...storeConfig, - persist: [ 'preferences' ], } ); -// We will be able to use the `register` function once we switch -// the "preferences" persistence to use the new preferences package. -const registeredStore = registerStore( STORE_NAME, { - ...storeConfig, - persist: [ 'preferences' ], -} ); -unlock( registeredStore ).registerPrivateActions( privateActions ); -unlock( registeredStore ).registerPrivateSelectors( privateSelectors ); - -// TODO: Remove once we switch to the `register` function (see above). -// -// Until then, private functions also need to be attached to the original -// `store` descriptor in order to avoid unit tests failing, which could happen -// when tests create new registries in which they register stores. -// -// @see https://github.com/WordPress/gutenberg/pull/51145#discussion_r1239999590 unlock( store ).registerPrivateActions( privateActions ); unlock( store ).registerPrivateSelectors( privateSelectors ); +register( store ); diff --git a/packages/block-editor/src/store/private-actions.js b/packages/block-editor/src/store/private-actions.js index 74aec3c49d1e8..9b65a173191bc 100644 --- a/packages/block-editor/src/store/private-actions.js +++ b/packages/block-editor/src/store/private-actions.js @@ -1,7 +1,9 @@ /** * WordPress dependencies */ +import { store as blocksStore } from '@wordpress/blocks'; import { Platform } from '@wordpress/element'; +import { store as preferencesStore } from '@wordpress/preferences'; /** * Internal dependencies @@ -382,3 +384,69 @@ export const modifyContentLockBlock = focusModeToRevert ); }; + +/** + * Updates the inserter usage statistics in the preferences store. + * + * Note: this function is an internal and not intended to ever be made + * non-private. + * + * @param {Array} blocks The array of blocks that were inserted. + */ +export const updateInsertUsage = + ( blocks ) => + ( { registry } ) => { + const previousInsertUsage = + registry.select( preferencesStore ).get( 'core', 'insertUsage' ) ?? + {}; + + const time = Date.now(); + + let updatedInsertUsage = blocks.reduce( ( previousState, block ) => { + const { attributes, name: blockName } = block; + let id = blockName; + const variation = registry + .select( blocksStore ) + .getActiveBlockVariation( blockName, attributes ); + + if ( variation?.name ) { + id += '/' + variation.name; + } + + if ( blockName === 'core/block' ) { + id += '/' + attributes.ref; + } + + const previousCount = previousState?.[ id ]?.count ?? 0; + + return { + ...previousState, + [ id ]: { + time, + count: previousCount + 1, + }, + }; + }, previousInsertUsage ); + + // Ensure the list of blocks doesn't grow above `limit` items. + // This is to ensure the preferences store data doesn't grow too big + // given it's persisted in the database. + const limit = 100; + const entries = Object.entries( updatedInsertUsage ); + if ( entries.length > limit ) { + // Most recently inserted blocks first. + entries.sort( ( a, b ) => b[ 1 ].time - a[ 1 ].time ); + + // Slice an array of items that are the newest and convert them + // back into object form. + const entriesToKeep = entries.slice( 0, limit ); + updatedInsertUsage = Object.fromEntries( entriesToKeep ); + } + + unlock( + registry.dispatch( preferencesStore ) + ).markNextChangeAsExpensive(); + registry + .dispatch( preferencesStore ) + .set( 'core', 'insertUsage', updatedInsertUsage ); + }; diff --git a/packages/block-editor/src/store/private-selectors.js b/packages/block-editor/src/store/private-selectors.js index c534c65b8defe..0f876ca02e854 100644 --- a/packages/block-editor/src/store/private-selectors.js +++ b/packages/block-editor/src/store/private-selectors.js @@ -2,6 +2,7 @@ * WordPress dependencies */ import { createSelector, createRegistrySelector } from '@wordpress/data'; +import { store as preferencesStore } from '@wordpress/preferences'; /** * Internal dependencies @@ -31,6 +32,8 @@ import { export { getBlockSettings } from './get-block-settings'; +const EMPTY_OBJECT = {}; + /** * Returns true if the block interface is hidden, or false otherwise. * @@ -511,3 +514,19 @@ export function getTemporarilyEditingAsBlocks( state ) { export function getTemporarilyEditingFocusModeToRevert( state ) { return state.temporarilyEditingFocusModeRevert; } + +/** + * Return all insert usage stats. + * + * This is only exported since registry selectors need to be exported. It's marked + * as unstable so that it's not considered part of the public API. + * + * @return {Object} An object with an `id` key representing the type + * of block and an object value that contains + * block insertion statistics. + */ +export const getInsertUsage = createRegistrySelector( + ( registrySelect ) => () => + registrySelect( preferencesStore ).get( 'core', 'insertUsage' ) ?? + EMPTY_OBJECT +); diff --git a/packages/block-editor/src/store/reducer.js b/packages/block-editor/src/store/reducer.js index 7c83887876919..1e8658e287d59 100644 --- a/packages/block-editor/src/store/reducer.js +++ b/packages/block-editor/src/store/reducer.js @@ -7,12 +7,12 @@ import fastDeepEqual from 'fast-deep-equal/es6'; * WordPress dependencies */ import { pipe } from '@wordpress/compose'; -import { combineReducers, select } from '@wordpress/data'; -import { store as blocksStore } from '@wordpress/blocks'; +import { combineReducers } from '@wordpress/data'; + /** * Internal dependencies */ -import { PREFERENCES_DEFAULTS, SETTINGS_DEFAULTS } from './defaults'; +import { SETTINGS_DEFAULTS } from './defaults'; import { insertAt, moveTo } from './array'; const identity = ( x ) => x; @@ -1676,58 +1676,6 @@ export function settings( state = SETTINGS_DEFAULTS, action ) { return state; } -/** - * Reducer returning the user preferences. - * - * @param {Object} state Current state. - * @param {Object} action Dispatched action. - * - * @return {string} Updated state. - */ -export function preferences( state = PREFERENCES_DEFAULTS, action ) { - switch ( action.type ) { - case 'INSERT_BLOCKS': - case 'REPLACE_BLOCKS': { - const nextInsertUsage = action.blocks.reduce( - ( prevUsage, block ) => { - const { attributes, name: blockName } = block; - let id = blockName; - // If a block variation match is found change the name to be the same with the - // one that is used for block variations in the Inserter (`getItemFromVariation`). - const match = select( blocksStore ).getActiveBlockVariation( - blockName, - attributes - ); - if ( match?.name ) { - id += '/' + match.name; - } - if ( blockName === 'core/block' ) { - id += '/' + attributes.ref; - } - - return { - ...prevUsage, - [ id ]: { - time: action.time, - count: prevUsage[ id ] - ? prevUsage[ id ].count + 1 - : 1, - }, - }; - }, - state.insertUsage - ); - - return { - ...state, - insertUsage: nextInsertUsage, - }; - } - } - - return state; -} - /** * Reducer returning an object where each key is a block client ID, its value * representing the settings for its nested blocks. @@ -2083,7 +2031,6 @@ const combinedReducers = combineReducers( { insertionPoint, template, settings, - preferences, lastBlockAttributesChange, lastFocus, editorMode, diff --git a/packages/block-editor/src/store/selectors.js b/packages/block-editor/src/store/selectors.js index d78d75e4210c8..b0b4ba0d17271 100644 --- a/packages/block-editor/src/store/selectors.js +++ b/packages/block-editor/src/store/selectors.js @@ -34,6 +34,7 @@ import { unlock } from '../lock-unlock'; import { getContentLockingParent, + getInsertUsage, getTemporarilyEditingAsBlocks, getTemporarilyEditingFocusModeToRevert, } from './private-selectors'; @@ -1819,20 +1820,6 @@ export function canLockBlockType( state, nameOrType ) { return !! state.settings?.canLockBlocks; } -/** - * Returns information about how recently and frequently a block has been inserted. - * - * @param {Object} state Global application state. - * @param {string} id A string which identifies the insert, e.g. 'core/block/12' - * - * @return {?{ time: number, count: number }} An object containing `time` which is when the last - * insert occurred as a UNIX epoch, and `count` which is - * the number of inserts that have occurred. - */ -function getInsertUsage( state, id ) { - return state.preferences.insertUsage?.[ id ] ?? null; -} - /** * Returns whether we can show a block type in the inserter * @@ -1859,7 +1846,8 @@ const canIncludeBlockTypeInInserter = ( state, blockType, rootClientId ) => { */ const getItemFromVariation = ( state, item ) => ( variation ) => { const variationId = `${ item.id }/${ variation.name }`; - const { time, count = 0 } = getInsertUsage( state, variationId ) || {}; + const insertUsage = getInsertUsage(); + const { time, count = 0 } = insertUsage?.[ variationId ] ?? {}; return { ...item, id: variationId, @@ -1934,7 +1922,8 @@ const buildBlockTypeItem = ).some( ( { name } ) => name === blockType.name ); } - const { time, count = 0 } = getInsertUsage( state, id ) || {}; + const insertUsage = getInsertUsage(); + const { time, count = 0 } = insertUsage?.[ id ] ?? {}; const blockItemBase = { id, name: blockType.name, @@ -2003,7 +1992,8 @@ export const getInserterItems = createRegistrySelector( ( select ) => } : symbol; const id = `core/block/${ reusableBlock.id }`; - const { time, count = 0 } = getInsertUsage( state, id ) || {}; + const insertUsage = getInsertUsage(); + const { time, count = 0 } = insertUsage?.[ id ] ?? {}; const frecency = calculateFrecency( time, count ); return { @@ -2147,7 +2137,7 @@ export const getInserterItems = createRegistrySelector( ( select ) => getBlockTypes(), unlock( select( STORE_NAME ) ).getReusableBlocks(), state.blocks.order, - state.preferences.insertUsage, + getInsertUsage(), ...getInsertBlockTypeDependants( state, rootClientId ), ] ) @@ -2214,7 +2204,7 @@ export const getBlockTransformItems = createSelector( }, ( state, blocks, rootClientId ) => [ getBlockTypes(), - state.preferences.insertUsage, + getInsertUsage(), ...getInsertBlockTypeDependants( state, rootClientId ), ] ); diff --git a/packages/block-editor/src/store/test/actions.js b/packages/block-editor/src/store/test/actions.js index f960363cdb0dd..abc0057afafea 100644 --- a/packages/block-editor/src/store/test/actions.js +++ b/packages/block-editor/src/store/test/actions.js @@ -219,6 +219,7 @@ describe( 'actions', () => { }; const dispatch = jest.fn(); dispatch.ensureDefaultBlock = jest.fn(); + dispatch.updateInsertUsage = jest.fn(); const registry = createRegistry(); replaceBlock( 'chicken', block )( { select, dispatch, registry } ); @@ -227,7 +228,6 @@ describe( 'actions', () => { type: 'REPLACE_BLOCKS', clientIds: [ 'chicken' ], blocks: [ block ], - time: expect.any( Number ), initialPosition: 0, } ); } ); @@ -260,6 +260,8 @@ describe( 'actions', () => { }, }; const dispatch = jest.fn(); + dispatch.ensureDefaultBlock = jest.fn(); + dispatch.updateInsertUsage = jest.fn(); replaceBlocks( [ 'chicken' ], blocks )( { select, dispatch } ); @@ -286,6 +288,7 @@ describe( 'actions', () => { }; const dispatch = jest.fn(); dispatch.ensureDefaultBlock = jest.fn(); + dispatch.updateInsertUsage = jest.fn(); const registry = createRegistry(); replaceBlocks( @@ -297,7 +300,6 @@ describe( 'actions', () => { type: 'REPLACE_BLOCKS', clientIds: [ 'chicken' ], blocks, - time: expect.any( Number ), initialPosition: 0, } ); } ); @@ -324,6 +326,7 @@ describe( 'actions', () => { }; const dispatch = jest.fn(); dispatch.ensureDefaultBlock = jest.fn(); + dispatch.updateInsertUsage = jest.fn(); const registry = createRegistry(); replaceBlocks( @@ -338,12 +341,45 @@ describe( 'actions', () => { type: 'REPLACE_BLOCKS', clientIds: [ 'chicken' ], blocks, - time: expect.any( Number ), indexToSelect: null, initialPosition: null, meta: { patternName: 'core/chicken-ribs-pattern' }, } ); } ); + + it( 'should set insertUsage in the preferences store', () => { + const blocks = [ + { + clientId: 'ribs', + name: 'core/test-ribs', + }, + { + clientId: 'chicken', + name: 'core/test-chicken', + }, + ]; + + const select = { + getSettings: () => null, + getBlockRootClientId: () => null, + canInsertBlockType: () => true, + getBlockCount: () => 1, + }; + + const dispatch = jest.fn(); + dispatch.ensureDefaultBlock = jest.fn(); + dispatch.updateInsertUsage = jest.fn(); + const registry = createRegistry(); + + replaceBlocks( + [ 'pineapple' ], + blocks, + null, + null + )( { select, dispatch, registry } ); + + expect( dispatch.updateInsertUsage ).toHaveBeenCalledWith( blocks ); + } ); } ); describe( 'insertBlock', () => { @@ -358,7 +394,9 @@ describe( 'actions', () => { getSettings: () => null, canInsertBlockType: () => true, }; - const dispatch = jest.fn(); + const dispatch = Object.assign( jest.fn(), { + updateInsertUsage: () => {}, + } ); insertBlock( block, @@ -372,7 +410,6 @@ describe( 'actions', () => { blocks: [ block ], index, rootClientId: 'testclientid', - time: expect.any( Number ), updateSelection: true, initialPosition: 0, } ); @@ -411,6 +448,8 @@ describe( 'actions', () => { }, }; const dispatch = jest.fn(); + dispatch.ensureDefaultBlock = jest.fn(); + dispatch.updateInsertUsage = jest.fn(); insertBlocks( blocks, @@ -424,7 +463,6 @@ describe( 'actions', () => { blocks: [ ribsBlock, chickenRibsBlock ], index: 5, rootClientId: 'testrootid', - time: expect.any( Number ), updateSelection: false, initialPosition: null, } ); @@ -446,6 +484,7 @@ describe( 'actions', () => { canInsertBlockType: () => false, }; const dispatch = jest.fn(); + dispatch.updateInsertUsage = jest.fn(); insertBlocks( blocks, @@ -489,6 +528,7 @@ describe( 'actions', () => { }, }; const dispatch = jest.fn(); + dispatch.updateInsertUsage = jest.fn(); insertBlocks( blocks, @@ -504,12 +544,41 @@ describe( 'actions', () => { blocks: [ ribsBlock, chickenRibsBlock ], index: 5, rootClientId: 'testrootid', - time: expect.any( Number ), updateSelection: false, initialPosition: null, meta: { patternName: 'core/chicken-ribs-pattern' }, } ); } ); + + it( 'should update insertUsage', () => { + const blocks = [ + { + clientId: 'ribs', + name: 'core/test-ribs', + }, + { + clientId: 'chicken', + name: 'core/test-chicken', + }, + ]; + + const select = { + getSettings: () => null, + canInsertBlockType: () => true, + }; + const dispatch = jest.fn(); + dispatch.updateInsertUsage = jest.fn(); + + insertBlocks( + blocks, + 5, + 'testrootid', + false, + 0 + )( { select, dispatch } ); + + expect( dispatch.updateInsertUsage ).toHaveBeenCalledWith( blocks ); + } ); } ); describe( 'showInsertionPoint', () => { @@ -783,7 +852,6 @@ describe( 'actions', () => { type: 'REPLACE_INNER_BLOCKS', blocks: [ block ], rootClientId: 'root', - time: expect.any( Number ), updateSelection: false, initialPosition: null, } ); @@ -794,7 +862,6 @@ describe( 'actions', () => { type: 'REPLACE_INNER_BLOCKS', blocks: [ block ], rootClientId: 'root', - time: expect.any( Number ), updateSelection: true, initialPosition: 0, } ); diff --git a/packages/block-editor/src/store/test/private-actions.js b/packages/block-editor/src/store/test/private-actions.js index 7576b95866306..5eb24c2e08dab 100644 --- a/packages/block-editor/src/store/test/private-actions.js +++ b/packages/block-editor/src/store/test/private-actions.js @@ -5,12 +5,15 @@ import { hideBlockInterface, showBlockInterface, expandBlock, + updateInsertUsage, __experimentalUpdateSettings, setOpenedBlockSettingsMenu, startDragging, stopDragging, } from '../private-actions'; +import { lock } from '../../lock-unlock'; + describe( 'private actions', () => { describe( 'hideBlockInterface', () => { it( 'should return the HIDE_BLOCK_INTERFACE action', () => { @@ -28,6 +31,165 @@ describe( 'private actions', () => { } ); } ); + describe( 'updateInsertUsage', () => { + it( 'should record recently used blocks', () => { + const actions = { set: jest.fn() }; + const privateActions = { markNextChangeAsExpensive: jest.fn() }; + lock( actions, privateActions ); + const registry = { + dispatch: () => actions, + select: () => ( { + get: () => {}, + getActiveBlockVariation: () => {}, + } ), + }; + + updateInsertUsage( [ + { + clientId: 'bacon', + name: 'core/embed', + }, + ] )( { registry } ); + + expect( + privateActions.markNextChangeAsExpensive + ).toHaveBeenCalled(); + expect( actions.set ).toHaveBeenCalledWith( 'core', 'insertUsage', { + 'core/embed': { + time: expect.any( Number ), + count: 1, + }, + } ); + } ); + + it( 'merges insert usage if more blocks are added of the same type', () => { + const actions = { set: jest.fn() }; + const privateActions = { markNextChangeAsExpensive: jest.fn() }; + lock( actions, privateActions ); + const registry = { + dispatch: () => actions, + select: () => ( { + // simulate an existing embed block. + get: () => ( { + 'core/embed': { + time: 123456, + count: 1, + }, + } ), + getActiveBlockVariation: () => {}, + } ), + }; + + updateInsertUsage( [ + { + clientId: 'eggs', + name: 'core/embed', + }, + { + clientId: 'bacon', + name: 'core/block', + attributes: { ref: 123 }, + }, + ] )( { registry } ); + + expect( + privateActions.markNextChangeAsExpensive + ).toHaveBeenCalled(); + expect( actions.set ).toHaveBeenCalledWith( 'core', 'insertUsage', { + // The reusable block has a special case where each ref is + // stored as though an individual block, and the ref is + // also recorded in the `insert` object. + 'core/block/123': { + time: expect.any( Number ), + count: 1, + }, + 'core/embed': { + time: expect.any( Number ), + count: 2, + }, + } ); + } ); + + describe( 'block variations handling', () => { + const blockWithVariations = 'core/test-block-with-variations'; + + it( 'should return proper results with both found or not found block variation matches', () => { + const actions = { set: jest.fn() }; + const privateActions = { markNextChangeAsExpensive: jest.fn() }; + lock( actions, privateActions ); + const registry = { + dispatch: () => actions, + select: () => ( { + get: () => {}, + // simulate an active block variation: + // - 'apple' when the fruit attribute is 'apple'. + // - 'orange' when the fruit attribute is 'orange'. + getActiveBlockVariation: ( + blockName, + { fruit } = {} + ) => { + if ( blockName === blockWithVariations ) { + if ( fruit === 'orange' ) { + return { name: 'orange' }; + } + if ( fruit === 'apple' ) { + return { name: 'apple' }; + } + } + }, + } ), + }; + + updateInsertUsage( [ + { + clientId: 'no match', + name: blockWithVariations, + }, + { + clientId: 'not a variation match', + name: blockWithVariations, + attributes: { fruit: 'not in a variation' }, + }, + { + clientId: 'orange', + name: blockWithVariations, + attributes: { fruit: 'orange' }, + }, + { + clientId: 'apple', + name: blockWithVariations, + attributes: { fruit: 'apple' }, + }, + ] )( { registry } ); + + const orangeVariationName = `${ blockWithVariations }/orange`; + const appleVariationName = `${ blockWithVariations }/apple`; + + expect( + privateActions.markNextChangeAsExpensive + ).toHaveBeenCalled(); + expect( actions.set ).toHaveBeenCalledWith( + 'core', + 'insertUsage', + { + [ orangeVariationName ]: { + time: expect.any( Number ), + count: 1, + }, + [ appleVariationName ]: { + time: expect.any( Number ), + count: 1, + }, + [ blockWithVariations ]: { + time: expect.any( Number ), + count: 2, + }, + } + ); + } ); + } ); + } ); + describe( '__experimentalUpdateSettings', () => { const experimentalSettings = { inserterMediaCategories: 'foo', diff --git a/packages/block-editor/src/store/test/reducer.js b/packages/block-editor/src/store/test/reducer.js index cd472fa59ac72..67408622357e7 100644 --- a/packages/block-editor/src/store/test/reducer.js +++ b/packages/block-editor/src/store/test/reducer.js @@ -26,7 +26,6 @@ import { selection, initialPosition, isMultiSelecting, - preferences, blocksMode, insertionPoint, template, @@ -2885,147 +2884,6 @@ describe( 'state', () => { } ); } ); - describe( 'preferences()', () => { - it( 'should apply all defaults', () => { - const state = preferences( undefined, {} ); - - expect( state ).toEqual( { - insertUsage: {}, - } ); - } ); - it( 'should record recently used blocks', () => { - const state = preferences( deepFreeze( { insertUsage: {} } ), { - type: 'INSERT_BLOCKS', - blocks: [ - { - clientId: 'bacon', - name: 'core/embed', - }, - ], - time: 123456, - } ); - - expect( state ).toEqual( { - insertUsage: { - 'core/embed': { - time: 123456, - count: 1, - }, - }, - } ); - - const twoRecentBlocks = preferences( - deepFreeze( { - insertUsage: { - 'core/embed': { - time: 123456, - count: 1, - }, - }, - } ), - { - type: 'INSERT_BLOCKS', - blocks: [ - { - clientId: 'eggs', - name: 'core/embed', - }, - { - clientId: 'bacon', - name: 'core/block', - attributes: { ref: 123 }, - }, - ], - time: 123457, - } - ); - - expect( twoRecentBlocks ).toEqual( { - insertUsage: { - 'core/embed': { - time: 123457, - count: 2, - }, - 'core/block/123': { - time: 123457, - count: 1, - }, - }, - } ); - } ); - describe( 'block variations handling', () => { - const blockWithVariations = 'core/test-block-with-variations'; - beforeAll( () => { - const variations = [ - { - name: 'apple', - attributes: { fruit: 'apple' }, - }, - { name: 'orange', attributes: { fruit: 'orange' } }, - ].map( ( variation ) => ( { - ...variation, - isActive: ( blockAttributes, variationAttributes ) => - blockAttributes?.fruit === variationAttributes.fruit, - } ) ); - registerBlockType( blockWithVariations, { - save: noop, - edit: noop, - title: 'Fruit with variations', - variations, - } ); - } ); - afterAll( () => { - unregisterBlockType( blockWithVariations ); - } ); - it( 'should return proper results with both found or not found block variation matches', () => { - const state = preferences( deepFreeze( { insertUsage: {} } ), { - type: 'INSERT_BLOCKS', - blocks: [ - { - clientId: 'no match', - name: blockWithVariations, - }, - { - clientId: 'not a variation match', - name: blockWithVariations, - attributes: { fruit: 'not in a variation' }, - }, - { - clientId: 'orange', - name: blockWithVariations, - attributes: { fruit: 'orange' }, - }, - { - clientId: 'apple', - name: blockWithVariations, - attributes: { fruit: 'apple' }, - }, - ], - time: 123456, - } ); - - const orangeVariationName = `${ blockWithVariations }/orange`; - const appleVariationName = `${ blockWithVariations }/apple`; - expect( state ).toEqual( { - insertUsage: expect.objectContaining( { - [ orangeVariationName ]: { - time: 123456, - count: 1, - }, - [ appleVariationName ]: { - time: 123456, - count: 1, - }, - [ blockWithVariations ]: { - time: 123456, - count: 2, - }, - } ), - } ); - } ); - } ); - } ); - describe( 'blocksMode', () => { it( 'should set mode to html if not set', () => { const action = { diff --git a/packages/block-editor/src/store/test/selectors.js b/packages/block-editor/src/store/test/selectors.js index 85006621c4701..4dc7411bc8d27 100644 --- a/packages/block-editor/src/store/test/selectors.js +++ b/packages/block-editor/src/store/test/selectors.js @@ -15,6 +15,7 @@ import { select, dispatch } from '@wordpress/data'; */ import * as selectors from '../selectors'; import { store } from '../'; +import { getInsertUsage } from '../private-selectors'; const { getBlockName, @@ -73,6 +74,11 @@ const { getBlockEditingMode, } = selectors; +jest.mock( '../private-selectors', () => ( { + ...jest.requireActual( '../private-selectors' ), + getInsertUsage: jest.fn(), +} ) ); + describe( 'selectors', () => { let cachedSelectors; @@ -3295,6 +3301,8 @@ describe( 'selectors', () => { } ); it( 'should properly list block type and reusable block items', async () => { + getInsertUsage.mockImplementation( () => ( {} ) ); + await dispatch( store ).updateSettings( { __experimentalReusableBlocks: [ { @@ -3348,6 +3356,10 @@ describe( 'selectors', () => { } ); it( 'should correctly cache the return values', async () => { + // Define the empty object here to simulate that the preferences + // store won't return a new object every time. + const EMPTY_OBJECT = {}; + getInsertUsage.mockImplementation( () => EMPTY_OBJECT ); await dispatch( store ).updateSettings( { __experimentalReusableBlocks: [ { @@ -3424,6 +3436,7 @@ describe( 'selectors', () => { } ); it( 'should set isDisabled when a block with `multiple: false` has been used', async () => { + getInsertUsage.mockImplementation( () => ( {} ) ); await dispatch( store ).resetBlocks( [ { clientId: 'block1', @@ -3439,16 +3452,9 @@ describe( 'selectors', () => { } ); it( 'should set a frecency', async () => { - for ( let i = 0; i < 10; i++ ) { - await dispatch( store ).insertBlocks( [ - { - clientId: 'block1', - name: 'core/test-block-b', - innerBlocks: [], - }, - ] ); - } - + getInsertUsage.mockImplementation( () => ( { + 'core/test-block-b': { count: 10, time: Date.now() }, + } ) ); const items = select( store ).getInserterItems(); const reusableBlock2Item = items.find( ( item ) => item.id === 'core/test-block-b' @@ -3686,6 +3692,10 @@ describe( 'selectors', () => { ); } ); it( 'should set frecency', () => { + getInsertUsage.mockImplementation( () => ( { + 'core/with-tranforms-a': { count: 10, time: 1000 }, + } ) ); + const state = { blocks: { byClientId: new Map(), diff --git a/packages/preferences-persistence/README.md b/packages/preferences-persistence/README.md index af8f6cc4b1438..7052de8eb25af 100644 --- a/packages/preferences-persistence/README.md +++ b/packages/preferences-persistence/README.md @@ -41,7 +41,9 @@ _Parameters_ - _options_ `Object`: - _options.preloadedData_ `?Object`: Any persisted preferences data that should be preloaded. When set, the persistence layer will avoid fetching data from the REST API. - _options.localStorageRestoreKey_ `?string`: The key to use for restoring the localStorage backup, used when the persistence layer calls `localStorage.getItem` or `localStorage.setItem`. -- _options.requestDebounceMS_ `?number`: Debounce requests to the API so that they only occur at minimum every `requestDebounceMS` milliseconds, and don't swamp the server. Defaults to 2500ms. +- _options.requestDebounceMS_ `?number`: Debounce requests to the API so that they only occur at minimum every `requestDebounceMS` milliseconds, and don't swamp the server. Defaults to 1000ms. +- _options.expensiveRequestDebounceMS_ `?number`: A longer debounce that can be defined for updates that have `isExpensive=true` defined. defaults to 5000ms. +- _options.maxWaitMS_ `?number`: The maximum wait, if the debouncing exceeds the wait, the callback will be invoked. _Returns_ diff --git a/packages/preferences-persistence/src/create/create-async-debouncer.js b/packages/preferences-persistence/src/create/create-async-debouncer.js new file mode 100644 index 0000000000000..64836d1440c9c --- /dev/null +++ b/packages/preferences-persistence/src/create/create-async-debouncer.js @@ -0,0 +1,73 @@ +/** + * Performs a trailing edge debounce of async functions. + * + * If the debounce is called twice at the same time: + * - the first is queued via a timeout on the first call. + * - the first timeout is cancelled and the second is queued on the second call. + * + * If the second call begins a promise resolution and a third call is made, + * the third call awaits resolution before queuing a third timeout. + * + * This is distinct from `{ debounce } from @wordpress/compose` in that it + * waits for promise resolution and is supports each debounce invocation + * having a variable delay. + * + * @param {Object} options + * @param {number} options.maxWaitMS + * @return {Function} A function that debounces the async function passed to it + * in the first parameter by the time passed in the second + * options parameter. + */ +export default function createAsyncDebouncer( { maxWaitMS } = {} ) { + let timeoutId; + let activePromise; + let firstRequestTime; + + return async function debounced( func, { delayMS } ) { + if ( activePromise ) { + // Let any active promises finish before queuing the next request. + await activePromise; + } + + // Clear any active timeouts, abandoning any requests that have + // been queued but not been made. + if ( timeoutId ) { + clearTimeout( timeoutId ); + timeoutId = null; + } + + // Trigger any trailing edge calls to the function. + return new Promise( ( resolve, reject ) => { + let maxedDelay = delayMS; + if ( maxWaitMS !== undefined ) { + if ( firstRequestTime ) { + const elapsed = Date.now() - firstRequestTime; + // Ensure wait is less than the maxWait. + maxedDelay = Math.max( 0, maxWaitMS - elapsed ); + // But also not longer than `delayMS`. + maxedDelay = Math.min( delayMS, maxedDelay ); + } else { + firstRequestTime = Date.now(); + } + } + + // Schedule the next request but with a delay. + timeoutId = setTimeout( () => { + activePromise = func() + .then( ( ...thenArgs ) => { + resolve( ...thenArgs ); + } ) + .catch( ( error ) => { + reject( error ); + } ) + .finally( () => { + // As soon this promise is complete, clear the way for the + // next function to be queued. + activePromise = null; + timeoutId = null; + firstRequestTime = null; + } ); + }, maxedDelay ); + } ); + }; +} diff --git a/packages/preferences-persistence/src/create/debounce-async.js b/packages/preferences-persistence/src/create/debounce-async.js deleted file mode 100644 index 89fb4c9b30705..0000000000000 --- a/packages/preferences-persistence/src/create/debounce-async.js +++ /dev/null @@ -1,75 +0,0 @@ -/** - * Performs a leading edge debounce of async functions. - * - * If three functions are throttled at the same time: - * - The first happens immediately. - * - The second is never called. - * - The third happens `delayMS` milliseconds after the first has resolved. - * - * This is distinct from `{ debounce } from @wordpress/compose` in that it - * waits for promise resolution. - * - * @param {Function} func A function that returns a promise. - * @param {number} delayMS A delay in milliseconds. - * - * @return {Function} A function that debounce whatever function is passed - * to it. - */ -export default function debounceAsync( func, delayMS ) { - let timeoutId; - let activePromise; - - return async function debounced( ...args ) { - // This is a leading edge debounce. If there's no promise or timeout - // in progress, call the debounced function immediately. - if ( ! activePromise && ! timeoutId ) { - return new Promise( ( resolve, reject ) => { - // Keep a reference to the promise. - activePromise = func( ...args ) - .then( ( ...thenArgs ) => { - resolve( ...thenArgs ); - } ) - .catch( ( error ) => { - reject( error ); - } ) - .finally( () => { - // As soon this promise is complete, clear the way for the - // next one to happen immediately. - activePromise = null; - } ); - } ); - } - - if ( activePromise ) { - // Let any active promises finish before queuing the next request. - await activePromise; - } - - // Clear any active timeouts, abandoning any requests that have - // been queued but not been made. - if ( timeoutId ) { - clearTimeout( timeoutId ); - timeoutId = null; - } - - // Trigger any trailing edge calls to the function. - return new Promise( ( resolve, reject ) => { - // Schedule the next request but with a delay. - timeoutId = setTimeout( () => { - activePromise = func( ...args ) - .then( ( ...thenArgs ) => { - resolve( ...thenArgs ); - } ) - .catch( ( error ) => { - reject( error ); - } ) - .finally( () => { - // As soon this promise is complete, clear the way for the - // next one to happen immediately. - activePromise = null; - timeoutId = null; - } ); - }, delayMS ); - } ); - }; -} diff --git a/packages/preferences-persistence/src/create/index.js b/packages/preferences-persistence/src/create/index.js index c550e0a7ac2c7..27c6753a30725 100644 --- a/packages/preferences-persistence/src/create/index.js +++ b/packages/preferences-persistence/src/create/index.js @@ -6,7 +6,7 @@ import apiFetch from '@wordpress/api-fetch'; /** * Internal dependencies */ -import debounceAsync from './debounce-async'; +import createAsyncDebouncer from './create-async-debouncer'; const EMPTY_OBJECT = {}; const localStorage = window.localStorage; @@ -16,25 +16,31 @@ const localStorage = window.localStorage; * REST API. * * @param {Object} options - * @param {?Object} options.preloadedData Any persisted preferences data that should be preloaded. - * When set, the persistence layer will avoid fetching data - * from the REST API. - * @param {?string} options.localStorageRestoreKey The key to use for restoring the localStorage backup, used - * when the persistence layer calls `localStorage.getItem` or - * `localStorage.setItem`. - * @param {?number} options.requestDebounceMS Debounce requests to the API so that they only occur at - * minimum every `requestDebounceMS` milliseconds, and don't - * swamp the server. Defaults to 2500ms. + * @param {?Object} options.preloadedData Any persisted preferences data that should be preloaded. + * When set, the persistence layer will avoid fetching data + * from the REST API. + * @param {?string} options.localStorageRestoreKey The key to use for restoring the localStorage backup, used + * when the persistence layer calls `localStorage.getItem` or + * `localStorage.setItem`. + * @param {?number} options.requestDebounceMS Debounce requests to the API so that they only occur at + * minimum every `requestDebounceMS` milliseconds, and don't + * swamp the server. Defaults to 1000ms. + * @param {?number} options.expensiveRequestDebounceMS A longer debounce that can be defined for updates that have + * `isExpensive=true` defined. defaults to 5000ms. + * @param {?number} options.maxWaitMS The maximum wait, if the debouncing exceeds the wait, the callback + * will be invoked. * * @return {Object} A persistence layer for WordPress user meta. */ export default function create( { preloadedData, localStorageRestoreKey = 'WP_PREFERENCES_RESTORE_DATA', - requestDebounceMS = 2500, + requestDebounceMS = 1000, + expensiveRequestDebounceMS = 5000, + maxWaitMS = 10000, } = {} ) { let cache = preloadedData; - const debouncedApiFetch = debounceAsync( apiFetch, requestDebounceMS ); + const debounce = createAsyncDebouncer( { maxWaitMS } ); async function get() { if ( cache ) { @@ -68,7 +74,7 @@ export default function create( { return cache; } - function set( newData ) { + function set( newData, { isExpensive = false } = {} ) { const dataWithTimestamp = { ...newData, _modified: new Date().toISOString(), @@ -83,26 +89,29 @@ export default function create( { JSON.stringify( dataWithTimestamp ) ); - // The user meta endpoint seems susceptible to errors when consecutive - // requests are made in quick succession. Ensure there's a gap between - // any consecutive requests. - // - // Catch and do nothing with errors from the REST API. - debouncedApiFetch( { - path: '/wp/v2/users/me', - method: 'PUT', - // `keepalive` will still send the request in the background, - // even when a browser unload event might interrupt it. - // This should hopefully make things more resilient. - // This does have a size limit of 64kb, but the data is usually - // much less. - keepalive: true, - data: { - meta: { - persisted_preferences: dataWithTimestamp, - }, - }, - } ).catch( () => {} ); + debounce( + () => + apiFetch( { + path: '/wp/v2/users/me', + method: 'PUT', + // `keepalive` will still send the request in the background, + // even when a browser unload event might interrupt it. + // This should hopefully make things more resilient. + // This does have a size limit of 64kb, but the data is usually + // much less. + keepalive: true, + data: { + meta: { + persisted_preferences: dataWithTimestamp, + }, + }, + } ).catch( () => {} ), + { + delayMS: isExpensive + ? expensiveRequestDebounceMS + : requestDebounceMS, + } + ); } return { diff --git a/packages/preferences-persistence/src/create/test/create-async-debouncer.js b/packages/preferences-persistence/src/create/test/create-async-debouncer.js new file mode 100644 index 0000000000000..33d54a9bc16c9 --- /dev/null +++ b/packages/preferences-persistence/src/create/test/create-async-debouncer.js @@ -0,0 +1,209 @@ +/** + * Internal dependencies + */ +import createAsyncDebouncer from '../create-async-debouncer'; + +// See https://stackoverflow.com/questions/52177631/jest-timer-and-promise-dont-work-well-settimeout-and-async-function. +// Jest fake timers and async functions don't mix too well, since queued up +// promises can prevent jest from calling timeouts. +// This function flushes promises in the queue. +function flushPromises() { + return new Promise( jest.requireActual( 'timers' ).setImmediate ); +} + +// Promisify a timeout for use with jest.fn. +function timeout( milliseconds ) { + return new Promise( ( resolve ) => setTimeout( resolve, milliseconds ) ); +} + +describe( 'debounceAsync', () => { + it( 'uses a trailing debounce by default, the first call happens after delayMS', async () => { + jest.useFakeTimers(); + const fn = jest.fn( async () => {} ); + const debounce = createAsyncDebouncer(); + debounce( () => fn(), { delayMS: 20 } ); + + // It isn't called immediately. + expect( fn ).not.toHaveBeenCalled(); + + await flushPromises(); + jest.advanceTimersByTime( 19 ); + + // It isn't called before `delayMS`. + expect( fn ).not.toHaveBeenCalled(); + + await flushPromises(); + jest.advanceTimersByTime( 2 ); + + // It is called after `delayMS`. + expect( fn ).toHaveBeenCalledTimes( 1 ); + + jest.runOnlyPendingTimers(); + jest.useRealTimers(); + } ); + + it( 'calls the function on the trailing edge only once when there are multiple trailing edge calls', async () => { + jest.useFakeTimers(); + const fn = jest.fn( async () => {} ); + const debounce = createAsyncDebouncer(); + + debounce( () => fn( 'A' ), { delayMS: 20 } ); + debounce( () => fn( 'B' ), { delayMS: 20 } ); + debounce( () => fn( 'C' ), { delayMS: 20 } ); + debounce( () => fn( 'D' ), { delayMS: 20 } ); + + await flushPromises(); + jest.runAllTimers(); + + expect( fn ).toHaveBeenCalledTimes( 1 ); + expect( fn ).toHaveBeenCalledWith( 'D' ); + + jest.runOnlyPendingTimers(); + jest.useRealTimers(); + } ); + + it( 'supports variable delay for each invocation of the debounce', async () => { + jest.useFakeTimers(); + const fn = jest.fn( async () => {} ); + const debounce = createAsyncDebouncer(); + + debounce( () => fn(), { delayMS: 5 } ); + + // Advance to just before the first delay. + await flushPromises(); + jest.advanceTimersByTime( 4 ); + + expect( fn ).toHaveBeenCalledTimes( 0 ); + + // Trigger a second shorter debounce + debounce( () => fn(), { delayMS: 2 } ); + + // Advance to just after the second delay. + await flushPromises(); + jest.advanceTimersByTime( 3 ); + + expect( fn ).toHaveBeenCalledTimes( 1 ); + + jest.runOnlyPendingTimers(); + jest.useRealTimers(); + } ); + + it( 'waits for promise resolution in the callback before debouncing again', async () => { + jest.useFakeTimers(); + + // The callback takes 10ms to resolve. + const fn = jest.fn( async () => timeout( 10 ) ); + const debounce = createAsyncDebouncer(); + + debounce( () => fn(), { delayMS: 20 } ); + + // Advance to just after delayMS, but before the callback has resolved. + await flushPromises(); + jest.advanceTimersByTime( 25 ); + + // The callback has started invoking but hasn't finished resolution + expect( fn ).toHaveBeenCalledTimes( 1 ); + + // Trigger another call. + debounce( () => fn(), { delayMS: 20 } ); + + // Advanced by enough to resolve the first timeout. + await flushPromises(); + jest.advanceTimersByTime( 10 ); + + expect( fn ).toHaveBeenCalledTimes( 1 ); + + // Then advance by enough to invoke the second timeout. + await flushPromises(); + jest.advanceTimersByTime( 20 ); + + // The second callback should have started but now be resolving. + expect( fn ).toHaveBeenCalledTimes( 2 ); + + jest.runOnlyPendingTimers(); + jest.useRealTimers(); + } ); + + it( 'invokes the callback when the maxWaitMS is reached, even when delayMS is still yet to elapse', async () => { + jest.useFakeTimers(); + const fn = jest.fn( async () => {} ); + const debounce = createAsyncDebouncer( { maxWaitMS: 8 } ); + + // The first call has been triggered, but will take 4ms to resolve. + debounce( () => fn(), { delayMS: 4 } ); + + // Advance by less than the delayMS (total time: 3ms). + await flushPromises(); + jest.advanceTimersByTime( 3 ); + expect( fn ).toHaveBeenCalledTimes( 0 ); + + // Trigger the debounce a second time, extending the delay + debounce( () => fn(), { delayMS: 4 } ); + + // Advance again by less than the delayMS (total time: 6ms). + await flushPromises(); + jest.advanceTimersByTime( 3 ); + expect( fn ).toHaveBeenCalledTimes( 0 ); + + // Trigger the debounce a third time, extending the delay + debounce( () => fn(), { delayMS: 4 } ); + + // Advance again by less than the delayMS, but this time the maxWait should + // cause an invocation of the callback. (total time: 9ms, max wait: 8ms). + await flushPromises(); + jest.advanceTimersByTime( 3 ); + expect( fn ).toHaveBeenCalledTimes( 1 ); + + jest.runOnlyPendingTimers(); + jest.useRealTimers(); + } ); + + it( 'is thenable, returning any data from promise resolution of the debounced function', async () => { + expect.assertions( 2 ); + const fn = async () => 'test'; + const debounce = createAsyncDebouncer(); + + // Test the return value via awaiting. + const returnValue = await debounce( () => fn(), { + delayMS: 1, + } ); + expect( returnValue ).toBe( 'test' ); + + // Test then-ing. + await debounce( () => fn(), { + delayMS: 1, + } ).then( ( thenValue ) => expect( thenValue ).toBe( 'test' ) ); + } ); + + it( 'is catchable', async () => { + expect.assertions( 2 ); + const expectedError = new Error( 'test' ); + const fn = async () => { + throw expectedError; + }; + + const debounce = createAsyncDebouncer(); + + // Test traditional try/catch. + try { + await debounce( () => fn(), { + delayMS: 1, + } ); + } catch ( error ) { + // Disable reason - the test uses `expect.assertions` to ensure + // conditional assertions are called. + // eslint-disable-next-line jest/no-conditional-expect + expect( error ).toBe( expectedError ); + } + + // Test chained .catch(). + await await debounce( () => fn(), { + delayMS: 1, + } ).catch( ( error ) => { + // Disable reason - the test uses `expect.assertions` to ensure + // conditional assertions are called. + // eslint-disable-next-line jest/no-conditional-expect + expect( error ).toBe( expectedError ); + } ); + } ); +} ); diff --git a/packages/preferences-persistence/src/create/test/debounce-async.js b/packages/preferences-persistence/src/create/test/debounce-async.js deleted file mode 100644 index 44a867c998fc0..0000000000000 --- a/packages/preferences-persistence/src/create/test/debounce-async.js +++ /dev/null @@ -1,129 +0,0 @@ -/** - * Internal dependencies - */ -import debounceAsync from '../debounce-async'; - -// See https://stackoverflow.com/questions/52177631/jest-timer-and-promise-dont-work-well-settimeout-and-async-function. -// Jest fake timers and async functions don't mix too well, since queued up -// promises can prevent jest from calling timeouts. -// This function flushes promises in the queue. -function flushPromises() { - return new Promise( jest.requireActual( 'timers' ).setImmediate ); -} - -// Promisify a timeout for use with jest.fn. -function timeout( milliseconds ) { - return new Promise( ( resolve ) => setTimeout( resolve, milliseconds ) ); -} - -describe( 'debounceAsync', () => { - it( 'uses a leading debounce, the first call happens immediately', () => { - const fn = jest.fn( async () => {} ); - const debounced = debounceAsync( fn, 20 ); - debounced(); - expect( fn ).toHaveBeenCalledTimes( 1 ); - } ); - - it( 'calls the function on the leading edge and then once on the trailing edge when there are multiple calls', async () => { - jest.useFakeTimers(); - const fn = jest.fn( async () => {} ); - const debounced = debounceAsync( fn, 20 ); - - debounced( 'A' ); - - expect( fn ).toHaveBeenCalledTimes( 1 ); - - debounced( 'B' ); - debounced( 'C' ); - debounced( 'D' ); - - await flushPromises(); - jest.runAllTimers(); - - expect( fn ).toHaveBeenCalledTimes( 2 ); - expect( fn ).toHaveBeenCalledWith( 'A' ); - expect( fn ).toHaveBeenCalledWith( 'D' ); - - jest.runOnlyPendingTimers(); - jest.useRealTimers(); - } ); - - it( 'ensures the delay has elapsed between calls', async () => { - jest.useFakeTimers(); - const fn = jest.fn( async () => timeout( 10 ) ); - const debounced = debounceAsync( fn, 20 ); - - // The first call has been triggered, but will take 10ms to resolve. - debounced(); - debounced(); - debounced(); - debounced(); - expect( fn ).toHaveBeenCalledTimes( 1 ); - - // The first call has resolved. The delay period has started but has yet to finish. - await flushPromises(); - jest.advanceTimersByTime( 11 ); - expect( fn ).toHaveBeenCalledTimes( 1 ); - - // The second call is about to commence, but hasn't yet. - await flushPromises(); - jest.advanceTimersByTime( 18 ); - expect( fn ).toHaveBeenCalledTimes( 1 ); - - // The second call has now commenced. - await flushPromises(); - jest.advanceTimersByTime( 2 ); - expect( fn ).toHaveBeenCalledTimes( 2 ); - - // No more calls happen. - await flushPromises(); - jest.runAllTimers(); - expect( fn ).toHaveBeenCalledTimes( 2 ); - - jest.runOnlyPendingTimers(); - jest.useRealTimers(); - } ); - - it( 'is thenable, returning any data from promise resolution of the debounced function', async () => { - expect.assertions( 2 ); - const fn = async () => 'test'; - const debounced = debounceAsync( fn, 20 ); - - // Test the return value via awaiting. - const returnValue = await debounced(); - expect( returnValue ).toBe( 'test' ); - - // Test then-ing. - await debounced().then( ( thenValue ) => - expect( thenValue ).toBe( 'test' ) - ); - } ); - - it( 'is catchable', async () => { - expect.assertions( 2 ); - const expectedError = new Error( 'test' ); - const fn = async () => { - throw expectedError; - }; - - const debounced = debounceAsync( fn, 20 ); - - // Test traditional try/catch. - try { - await debounced(); - } catch ( error ) { - // Disable reason - the test uses `expect.assertions` to ensure - // conditional assertions are called. - // eslint-disable-next-line jest/no-conditional-expect - expect( error ).toBe( expectedError ); - } - - // Test chained .catch(). - await debounced().catch( ( error ) => { - // Disable reason - the test uses `expect.assertions` to ensure - // conditional assertions are called. - // eslint-disable-next-line jest/no-conditional-expect - expect( error ).toBe( expectedError ); - } ); - } ); -} ); diff --git a/packages/preferences-persistence/src/create/test/index.js b/packages/preferences-persistence/src/create/test/index.js index acf28a9c51ff0..3eb0c0a666662 100644 --- a/packages/preferences-persistence/src/create/test/index.js +++ b/packages/preferences-persistence/src/create/test/index.js @@ -10,6 +10,14 @@ import create from '..'; jest.mock( '@wordpress/api-fetch' ); +// See https://stackoverflow.com/questions/52177631/jest-timer-and-promise-dont-work-well-settimeout-and-async-function. +// Jest fake timers and async functions don't mix too well, since queued up +// promises can prevent jest from calling timeouts. +// This function flushes promises in the queue. +function flushPromises() { + return new Promise( jest.requireActual( 'timers' ).setImmediate ); +} + describe( 'create', () => { afterEach( () => { apiFetch.mockReset(); @@ -39,7 +47,10 @@ describe( 'create', () => { ); } ); - it( 'sends data to the `users/me` endpoint', () => { + it( 'sends data to the `users/me` endpoint', async () => { + // Take control of time, since `set` implements a debounce. + jest.useFakeTimers(); + apiFetch.mockResolvedValueOnce(); const { set } = create(); @@ -47,6 +58,9 @@ describe( 'create', () => { const data = { test: 1 }; set( data ); + await flushPromises(); + jest.runAllTimers(); + expect( apiFetch ).toHaveBeenCalledWith( { path: '/wp/v2/users/me', method: 'PUT', @@ -57,6 +71,9 @@ describe( 'create', () => { }, }, } ); + + jest.runOnlyPendingTimers(); + jest.useRealTimers(); } ); } ); @@ -75,6 +92,8 @@ describe( 'create', () => { } ); it( 'returns from a local cache once `set` has been called', async () => { + // Take control of time, since `set` implements a debounce. + jest.useFakeTimers(); const getItemSpy = jest.spyOn( global.Storage.prototype, 'getItem' @@ -86,6 +105,10 @@ describe( 'create', () => { // apiFetch was called as a result of calling `set`. set( data ); + + await flushPromises(); + jest.runAllTimers(); + expect( apiFetch ).toHaveBeenCalled(); apiFetch.mockClear(); @@ -94,6 +117,9 @@ describe( 'create', () => { expect( await get() ).toEqual( expect.objectContaining( data ) ); expect( getItemSpy ).not.toHaveBeenCalled(); expect( apiFetch ).not.toHaveBeenCalled(); + + jest.runOnlyPendingTimers(); + jest.useRealTimers(); } ); it( 'returns data from the users/me endpoint if there is no data in localStorage', async () => { diff --git a/packages/preferences-persistence/src/index.js b/packages/preferences-persistence/src/index.js index 2b1bcdd10fb03..68ffe4e41dcde 100644 --- a/packages/preferences-persistence/src/index.js +++ b/packages/preferences-persistence/src/index.js @@ -2,7 +2,11 @@ * Internal dependencies */ import create from './create'; -import convertLegacyLocalStorageData from './migrations/legacy-local-storage-data'; +import { + getLegacyData, + convertLegacyData, + convertLegacyInsertUsageData, +} from './migrations/legacy-local-storage-data'; import convertPreferencesPackageData from './migrations/preferences-package-data'; export { create }; @@ -38,9 +42,31 @@ export function __unstableCreatePersistenceLayer( serverData, userId ) { preloadedData = convertPreferencesPackageData( serverData ); } else if ( localData ) { preloadedData = convertPreferencesPackageData( localData ); - } else { + } + + // Handle legacy data migrations. + if ( ! preloadedData ) { // Check if there is data in the legacy format from the old persistence system. - preloadedData = convertLegacyLocalStorageData( userId ); + const legacyData = getLegacyData( userId ); + preloadedData = convertLegacyData( legacyData ); + } + + // The insertUsage preference was migrated later than others, the following code + // performs a separate migration just for this data. + if ( preloadedData && ! preloadedData?.core?.insertUsage ) { + const legacyData = getLegacyData( userId ); + + // Only run the migration if there's something to migrate. + if ( legacyData?.[ 'core/block-editor' ]?.preferences?.insertUsage ) { + // Check if there is data in the legacy format from the old persistence system. + preloadedData = { + ...preloadedData, + core: { + ...preloadedData.core, + insertUsage: convertLegacyInsertUsageData( legacyData ), + }, + }; + } } return create( { diff --git a/packages/preferences-persistence/src/migrations/legacy-local-storage-data/index.js b/packages/preferences-persistence/src/migrations/legacy-local-storage-data/index.js index 6f0bebd13a8a9..66022c554f123 100644 --- a/packages/preferences-persistence/src/migrations/legacy-local-storage-data/index.js +++ b/packages/preferences-persistence/src/migrations/legacy-local-storage-data/index.js @@ -14,7 +14,7 @@ import convertEditPostPanels from './convert-edit-post-panels'; * * @return {Object | null} The local storage data. */ -function getLegacyData( userId ) { +export function getLegacyData( userId ) { const key = `WP_DATA_USER_${ userId }`; const unparsedData = window.localStorage.getItem( key ); return JSON.parse( unparsedData ); @@ -81,22 +81,23 @@ export function convertLegacyData( data ) { { from: 'core/edit-site', to: 'core/edit-site' }, 'editorMode' ); + data = moveIndividualPreference( + data, + { from: 'core/block-editor', to: 'core' }, + 'insertUsage' + ); // The new system is only concerned with persisting // 'core/preferences' preferences reducer, so only return that. return data?.[ 'core/preferences' ]?.preferences; } -/** - * Gets the legacy local storage data for the given user and returns the - * data converted to the new format. - * - * @param {string | number} userId The user id. - * - * @return {Object | undefined} The converted data or undefined if no local - * storage data could be found. - */ -export default function convertLegacyLocalStorageData( userId ) { - const data = getLegacyData( userId ); - return convertLegacyData( data ); +export function convertLegacyInsertUsageData( data ) { + data = moveIndividualPreference( + data, + { from: 'core/block-editor', to: 'core' }, + 'insertUsage' + ); + + return data?.[ 'core/preferences' ]?.preferences?.core?.insertUsage; } diff --git a/packages/preferences-persistence/src/migrations/legacy-local-storage-data/test/index.js b/packages/preferences-persistence/src/migrations/legacy-local-storage-data/test/index.js index d4df39165c73d..87709791075e1 100644 --- a/packages/preferences-persistence/src/migrations/legacy-local-storage-data/test/index.js +++ b/packages/preferences-persistence/src/migrations/legacy-local-storage-data/test/index.js @@ -4,6 +4,40 @@ import { convertLegacyData } from '..'; const legacyData = { + 'core/block-editor': { + preferences: { + insertUsage: { + 'core/paragraph': { + time: 1649320988011, + count: 2, + insert: { + name: 'core/paragraph', + }, + }, + 'core/quote': { + time: 1649320934860, + count: 1, + insert: { + name: 'core/quote', + }, + }, + 'core/image': { + time: 1649321017053, + count: 1, + insert: { + name: 'core/image', + }, + }, + 'core/group': { + time: 1649321017077, + count: 1, + insert: { + name: 'core/group', + }, + }, + }, + }, + }, 'core/interface': { enableItems: { singleEnableItems: { @@ -65,42 +99,44 @@ const legacyData = { }; const alreadyConvertedData = { - 'core/block-editor': { + 'core/preferences': { preferences: { - insertUsage: { - 'core/paragraph': { - time: 1649320988011, - count: 2, - insert: { - name: 'core/paragraph', + core: { + insertUsage: { + 'core/paragraph': { + time: 1649320988011, + count: 2, + insert: { + name: 'core/paragraph', + }, }, - }, - 'core/quote': { - time: 1649320934860, - count: 1, - insert: { - name: 'core/quote', + 'core/quote': { + time: 1649320934860, + count: 1, + insert: { + name: 'core/quote', + }, }, - }, - 'core/image': { - time: 1649321017053, - count: 1, - insert: { - name: 'core/image', + 'core/image': { + time: 1649321017053, + count: 1, + insert: { + name: 'core/image', + }, }, - }, - 'core/group': { - time: 1649321017077, - count: 1, - insert: { - name: 'core/group', + 'core/group': { + time: 1649321017077, + count: 1, + insert: { + name: 'core/group', + }, }, }, }, - }, - }, - 'core/preferences': { - preferences: { + 'core/customize-widgets': { + welcomeGuide: false, + fixedToolbar: true, + }, 'core/edit-widgets': { welcomeGuide: false, fixedToolbar: true, @@ -134,6 +170,38 @@ describe( 'convertLegacyData', () => { it( 'converts to the expected format', () => { expect( convertLegacyData( legacyData ) ).toMatchInlineSnapshot( ` { + "core": { + "insertUsage": { + "core/group": { + "count": 1, + "insert": { + "name": "core/group", + }, + "time": 1649321017077, + }, + "core/image": { + "count": 1, + "insert": { + "name": "core/image", + }, + "time": 1649321017053, + }, + "core/paragraph": { + "count": 2, + "insert": { + "name": "core/paragraph", + }, + "time": 1649320988011, + }, + "core/quote": { + "count": 1, + "insert": { + "name": "core/quote", + }, + "time": 1649320934860, + }, + }, + }, "core/customize-widgets": { "fixedToolbar": true, "keepCaretInsideBlock": true, @@ -183,6 +251,42 @@ describe( 'convertLegacyData', () => { expect( convertLegacyData( alreadyConvertedData ) ) .toMatchInlineSnapshot( ` { + "core": { + "insertUsage": { + "core/group": { + "count": 1, + "insert": { + "name": "core/group", + }, + "time": 1649321017077, + }, + "core/image": { + "count": 1, + "insert": { + "name": "core/image", + }, + "time": 1649321017053, + }, + "core/paragraph": { + "count": 2, + "insert": { + "name": "core/paragraph", + }, + "time": 1649320988011, + }, + "core/quote": { + "count": 1, + "insert": { + "name": "core/quote", + }, + "time": 1649320934860, + }, + }, + }, + "core/customize-widgets": { + "fixedToolbar": true, + "welcomeGuide": false, + }, "core/edit-post": { "complementaryArea": "edit-post/block", "editorMode": "visual", diff --git a/packages/preferences/src/store/index.js b/packages/preferences/src/store/index.js index 7e57dac5e712c..215a8849fcb4b 100644 --- a/packages/preferences/src/store/index.js +++ b/packages/preferences/src/store/index.js @@ -8,8 +8,10 @@ import { createReduxStore, register } from '@wordpress/data'; */ import reducer from './reducer'; import * as actions from './actions'; +import * as privateActions from './private-actions'; import * as selectors from './selectors'; import { STORE_NAME } from './constants'; +import { unlock } from '../lock-unlock'; /** * Store definition for the preferences namespace. @@ -24,4 +26,5 @@ export const store = createReduxStore( STORE_NAME, { selectors, } ); +unlock( store ).registerPrivateActions( privateActions ); register( store ); diff --git a/packages/preferences/src/store/private-actions.js b/packages/preferences/src/store/private-actions.js new file mode 100644 index 0000000000000..86a72b3d75003 --- /dev/null +++ b/packages/preferences/src/store/private-actions.js @@ -0,0 +1,12 @@ +/** + * Marks the next value change as 'expensive'. + * + * This can be used for preferences that might be updated regularly. + * The persistence layer `set` function receives an `isExpensive: true` option when + * this is set and allows it to make optimizations around saving the value. + * + * @return {Object} Action object. + */ +export const markNextChangeAsExpensive = () => ( { + type: 'MARK_NEXT_CHANGE_AS_EXPENSIVE', +} ); diff --git a/packages/preferences/src/store/reducer.js b/packages/preferences/src/store/reducer.js index 0d2f463ab01de..3b1f12f989da0 100644 --- a/packages/preferences/src/store/reducer.js +++ b/packages/preferences/src/store/reducer.js @@ -41,6 +41,7 @@ export function defaults( state = {}, action ) { */ function withPersistenceLayer( reducer ) { let persistenceLayer; + let isNextChangeExpensive = false; return ( state, action ) => { // Setup the persistence layer, and return the persisted data @@ -51,9 +52,16 @@ function withPersistenceLayer( reducer ) { return persistedData; } + if ( action.type === 'MARK_NEXT_CHANGE_AS_EXPENSIVE' ) { + isNextChangeExpensive = true; + } + const nextState = reducer( state, action ); if ( action.type === 'SET_PREFERENCE_VALUE' ) { - persistenceLayer?.set( nextState ); + persistenceLayer?.set( nextState, { + isExpensive: isNextChangeExpensive, + } ); + isNextChangeExpensive = false; } return nextState; diff --git a/packages/preferences/src/store/test/reducer.js b/packages/preferences/src/store/test/reducer.js index 985959acb2f00..60919bd5ebc00 100644 --- a/packages/preferences/src/store/test/reducer.js +++ b/packages/preferences/src/store/test/reducer.js @@ -42,6 +42,45 @@ describe( 'withPersistenceLayer( preferences )', () => { const state = preferences( {}, setPreferenceValueAction ); - expect( set ).toHaveBeenCalledWith( state ); + expect( set ).toHaveBeenCalledWith( state, { isExpensive: false } ); + } ); + + it( 'passes the `isExpensive` flag to the persistence layer when the `MARK_NEXT_CHANGE_AS_EXPENSIVE` action is dispatched', () => { + const set = jest.fn(); + const persistenceLayer = { + set, + }; + + // Set the persistence layer. + const setPersistenceLayerAction = { + type: 'SET_PERSISTENCE_LAYER', + persistenceLayer, + persistedData: {}, + }; + preferences( {}, setPersistenceLayerAction ); + + // Mark the next change as expensive. + const markNextChangeAsExpensiveAction = { + type: 'MARK_NEXT_CHANGE_AS_EXPENSIVE', + }; + preferences( {}, markNextChangeAsExpensiveAction ); + + // Update a value, and expect the persistence layer `set` function to receive the `isExpensive: true` option. + const setPreferenceValueAction1 = { + type: 'SET_PREFERENCE_VALUE', + name: 'myPreference', + value: 'myValue1', + }; + const state1 = preferences( {}, setPreferenceValueAction1 ); + expect( set ).toHaveBeenCalledWith( state1, { isExpensive: true } ); + + // Update with another value, and expect that the `isExpensive` option is `false` again. + const setPreferenceValueAction2 = { + type: 'SET_PREFERENCE_VALUE', + name: 'myPreference', + value: 'myValue2', + }; + const state2 = preferences( {}, setPreferenceValueAction2 ); + expect( set ).toHaveBeenCalledWith( state2, { isExpensive: false } ); } ); } ); diff --git a/packages/private-apis/src/implementation.js b/packages/private-apis/src/implementation.js index c268e46f669cb..a95ec5d0b32e1 100644 --- a/packages/private-apis/src/implementation.js +++ b/packages/private-apis/src/implementation.js @@ -31,6 +31,7 @@ const CORE_MODULES_USING_PRIVATE_APIS = [ '@wordpress/reusable-blocks', '@wordpress/router', '@wordpress/dataviews', + '@wordpress/preferences', ]; /** diff --git a/packages/reusable-blocks/src/store/test/actions.js b/packages/reusable-blocks/src/store/test/actions.js index b2feae1195ee1..9db00c25f213c 100644 --- a/packages/reusable-blocks/src/store/test/actions.js +++ b/packages/reusable-blocks/src/store/test/actions.js @@ -11,6 +11,7 @@ import { } from '@wordpress/blocks'; import { store as coreStore } from '@wordpress/core-data'; +import { store as preferencesStore } from '@wordpress/preferences'; import apiFetch from '@wordpress/api-fetch'; /** @@ -31,6 +32,7 @@ function createRegistryWithStores() { registry.register( blockEditorStore ); registry.register( reusableBlocksStore ); registry.register( blocksStore ); + registry.register( preferencesStore ); // Register entity here instead of mocking API handlers for loadPostTypeEntities() registry.dispatch( coreStore ).addEntities( [ diff --git a/test/e2e/specs/editor/various/navigable-toolbar.spec.js b/test/e2e/specs/editor/various/navigable-toolbar.spec.js index 4cfc3f9c21008..41862b4cdea94 100644 --- a/test/e2e/specs/editor/various/navigable-toolbar.spec.js +++ b/test/e2e/specs/editor/various/navigable-toolbar.spec.js @@ -135,6 +135,7 @@ test.describe( 'Block Toolbar', () => { BlockToolbarUtils, page, pageUtils, + requestUtils, } ) => { /* eslint-disable playwright/expect-expect */ /* eslint-disable playwright/no-wait-for-timeout */ @@ -166,8 +167,8 @@ test.describe( 'Block Toolbar', () => { await BlockToolbarUtils.testScrollable( blockToolbar, blockButton ); // Test cleanup - await editor.setIsFixedToolbar( false ); await pageUtils.setBrowserViewport( 'large' ); + await requestUtils.resetPreferences(); /* eslint-enable playwright/expect-expect */ /* eslint-enable playwright/no-wait-for-timeout */ } ); @@ -177,6 +178,7 @@ test.describe( 'Block Toolbar', () => { BlockToolbarUtils, page, pageUtils, + requestUtils, } ) => { // On default floating toolbar await editor.insertBlock( { name: 'core/paragraph' } ); @@ -236,8 +238,8 @@ test.describe( 'Block Toolbar', () => { await BlockToolbarUtils.expectLabelToHaveFocus( 'Block: Paragraph' ); // Test cleanup - await editor.setIsFixedToolbar( false ); await pageUtils.setBrowserViewport( 'large' ); + await requestUtils.resetPreferences(); } ); test( 'Focus should remain on mover when moving blocks', async ( { diff --git a/test/e2e/specs/site-editor/activate-theme.spec.js b/test/e2e/specs/site-editor/activate-theme.spec.js index a5fb4b3be0b73..c445c967836f5 100644 --- a/test/e2e/specs/site-editor/activate-theme.spec.js +++ b/test/e2e/specs/site-editor/activate-theme.spec.js @@ -36,6 +36,12 @@ test.describe( 'Activate theme', () => { await expect( page.locator( '.edit-site-canvas-loader' ) ).toHaveCount( 0 ); + // Usually `visitSiteEditor` is used to test the site editor, and it + // hides the welcome guide. Since this test uses a different flow, + // manually update the preferences here. + await editor.setPreferences( 'core/edit-site', { + welcomeGuide: false, + } ); await editor.canvas.locator( 'body' ).click(); await page .getByRole( 'region', { name: 'Editor top bar' } )