Migrate block editor insert usage to preferences store #39632
Conversation
talldan
added
[Type] Enhancement
talldan
requested review from
ellatrix,
nerrad,
ajitbohra,
mkaz,
ryanwelcher and
juanmaguitar
as code owners
talldan
force-pushed
the
migrate/block-editor-insert-usage-to-preferences-store
branch
from
ff58d97
to
6598078
Compare
|
Size Change: +585 B (+0.03%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
|
Looks like there are some failing tests - some more unit tests that need various mocks in place now that they touch the preferences store. I'll update those tomorrow. |
|
There's a discussion in #39672 (comment) about whether |
|
Thanks for the review! I'll hold off on merging this and switch it to a draft until this conversation about adding preferences as a dependency of block-editor (#39672 (comment)) is resolved. |
talldan
force-pushed
the
migrate/block-editor-insert-usage-to-preferences-store
branch
from
dd14c00
to
35c4e0c
Compare
talldan
force-pushed
the
migrate/block-editor-insert-usage-to-preferences-store
branch
from
35c4e0c
to
6e3679b
Compare
|
Status update on this one, this conversation is very relevant to this PR - #39795 (comment). Now that preferences are saved via the REST API, merging this would mean that every block insert would result in an API request, which is probably undesirable. I'm going to be AFK for a month so won't be able to solve this one quickly, but happy to look at it when I return. |
talldan
force-pushed
the
migrate/block-editor-insert-usage-to-preferences-store
branch
from
6e3679b
to
6d99419
Compare
talldan
force-pushed
the
migrate/block-editor-insert-usage-to-preferences-store
branch
from
6d99419
to
9cc9e08
Compare
talldan
force-pushed
the
migrate/block-editor-insert-usage-to-preferences-store
branch
from
67aadf8
to
7367bdb
Compare
Yeah, I don't like it, the reason it's there is the usage in the export const getInserterItems = createSelector(
( state, rootClientId = null ) => {
// ...
},
( state ) => [
// ...,
__unstableGetInsertUsage(),
]
);I don't think there's a way to call My understanding is that this PR will unlock the ability to use private registry selectors in the block editor store, so I've switched it to being a private registry selector called I don't know if there's another way around this, but this provides some nice symmetry with |
talldan
force-pushed
the
migrate/block-editor-insert-usage-to-preferences-store
branch
from
4fc359d
to
353c350
Compare
I tried limiting it to the 100 most recently inserted blocks. I did it this way so that if a person goes away for three months, and then comes back they still have some I think 100 should be enough to not have an adverse effect on the frecency. I did also consider only keeping the 100 most frecent blocks, but this causes an issue in that new blocks can never get into an established top 100 (since they need some existing Since the slicing happens in I think I've addressed all the review feedback now. |
| @@ -2008,7 +1996,8 @@ export const getInserterItems = createSelector( | |||
| } | |||
|
|
|||
| 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 ); | |||
There was a problem hiding this comment.
The getInsertUsage and calculateFrecency calls could be one day combined into one getBlockFrecency( id ) selector. They are always used together, there's no other use of the insert usage data than calculating frecency for a particular block.
| // This is a leading edge debounce. If there's no promise or timeout | ||
| // in progress, call the debounced function immediately. | ||
| if ( ! activePromise && ! timeoutId ) { | ||
| if ( ! isTrailing && ! activePromise && ! timeoutId ) { |
There was a problem hiding this comment.
I think this debounce logic has been a little bit flawed from the beginning, and the long expensiveRequestDebounceMS makes it worse.
If I call the debounced function repeatedly, the timeout keeps getting cancelled and rescheduled, and as a result the save can be never performed. Or only after a very long time, much longer than the intended timeout.
To avoid this "starvation" problem, we should do throttling rather than debouncing. Once the 60sec timeout is active, keep it running, but on further calls only update the function to be called when it finally expires.
The Lodash debounce function has a maxWait option that achieves something similar.
If we fix this behavior, then maybe we can remove also the invocation on the leading edge? I don't know why it was originally added -- why can't we always wait 2.5s? Maybe it's a workaround for this starvation problem?
There was a problem hiding this comment.
Just noting that we already have both throttle and debounce utilities as part of the @wordpress/compose package, and the built-in debounce() also supports a maxWait option. Just in case that could be something you'd like to utilize instead of a custom debounce logic.
There was a problem hiding this comment.
If I call the debounced function repeatedly, the timeout keeps getting cancelled and rescheduled, and as a result the save can be never performed. Or only after a very long time, much longer than the intended timeout.
To avoid this "starvation" problem, we should do throttling rather than debouncing. Once the 60sec timeout is active, keep it running, but on further calls only update the function to be called when it finally expires.
I'll look into it. It's not really much of an issue with the shorter delay, but perhaps becomes more apparent with the longer delay. 🤔
The main aim is to avoid repeated REST requests. WordPress servers are very bad at handling multiple requests at the same time, but perhaps it's ok to throttle.
If we fix this behavior, then maybe we can remove also the invocation on the leading edge? I don't know why it was originally added -- why can't we always wait 2.5s? Maybe it's a workaround for this starvation problem?
The most common use case is infrequent preferences calls, so triggering the request right away is usually fine. It also reduces the chances that the delay/request might be in flight while the user unloads the page, causing the request to never go through.
Just noting that we already have both throttle and debounce utilities as part of the @wordpress/compose package, and the built-in debounce() also supports a maxWait option. Just in case that could be something you'd like to utilize instead of a custom debounce logic.
@tyxla It needs to support async functions, which most libraries don't. The code here ensures requests are never made in parallel—it delays from after a promise completion rather than from function invocation. It might be there's another way of doing this that allows leverage of worpdress/compose, any suggestions appreciated.
There was a problem hiding this comment.
I've updated it to work as @jsnajdr describes so that we can try it out.
I think it works well. I've kept the debounce for the non-expensive call a short 1 second, and the expensive one 5 seconds, and there's a 10 second max wait.
The values feel a bit arbitrary, but I figure it should be ok to prevent spamming of HTTP requests if the user is duplicating lots of blocks.
The function is now also very simple, which is a bonus.
I think all the other feedback has also been addressed. I still need to give this a thorough test as so long has passed since the last time I looked at this PR.
…ordPress#52088) As a workaround, until WordPress#39632 is merged, make sure that private actions and selectors can be unlocked from the original store descriptor (the one created by `createReduxStore`) and not just the one registered in the default registry (created by `registerStore`). Without this workaround, specific setups will unexpectedly fail, such as the action tests in the `reusable-blocks` package, due to the way that those tests create their own registries in which they register stores like `block-editor`. Context: WordPress#51145 (comment) Props jsnajdr
…52088) As a workaround, until #39632 is merged, make sure that private actions and selectors can be unlocked from the original store descriptor (the one created by `createReduxStore`) and not just the one registered in the default registry (created by `registerStore`). Without this workaround, specific setups will unexpectedly fail, such as the action tests in the `reusable-blocks` package, due to the way that those tests create their own registries in which they register stores like `block-editor`. Context: #51145 (comment) Props jsnajdr
* Try restoring the site editor animation (#51956) * Try restoring the site editor animation * fix header animation * Remove accidental addition of layout prop * tidy up formatting * fix animate presence issue * Fix animation between sidebar view and distraction free edit view * Leave sidebar present and maintain canvas to sidebar animation The sidebar is necessary for routing on mobile so we have to maintain its presence in the DOM. Just hiding it isn't enough though, as it is still able to be reached with keyboard tabs and screen readers. Using the relatively new `inert` property disables the element from user interaction, so we add that when we don't want the sidebar to be shown. * Fix mobile view for pattern library On Mobile, the canvas mode wasn't being set to edit when using the pattern library. This updates it to use the showSidbar value instead, keeping it in sync with the inert setting. --------- Co-authored-by: Saxon Fletcher <[email protected]> Co-authored-by: Jerry Jones <[email protected]> * Change password input to type text so contents are visible. (#52622) * Iframe: Silence style compat warnings when in a BlockPreview (#52627) * Do not autofocus page title field in the Create new page modal dialog. (#52603) * Use lowercase p in "Manage Patterns" (#52617) * Remove theme patterns title (#52570) * Block editor store: also attach private APIs to old store descriptor (#52088) As a workaround, until #39632 is merged, make sure that private actions and selectors can be unlocked from the original store descriptor (the one created by `createReduxStore`) and not just the one registered in the default registry (created by `registerStore`). Without this workaround, specific setups will unexpectedly fail, such as the action tests in the `reusable-blocks` package, due to the way that those tests create their own registries in which they register stores like `block-editor`. Context: #51145 (comment) Props jsnajdr * Block removal prompt: let consumers pass their own rules (#51841) * Block removal prompt: let consumers pass their own rules Following up on #51145, this untangles `edit-site` from `block-editor` by removing the hard-coded set of rules `blockTypePromptMessages` from the generic `BlockRemovalWarningModal` component. Rules are now to be passed to that component by whichever block editor is using it. Names and comments have been updated accordingly and improved. * Site editor: Add e2e test for block removal prompt * Fix Shift+Tab to Block Toolbar (#52613) * Fix Shift+Tab to Block Toolbar * Add changelog entry * Show warning on removal of Post Template block in the site editor. (#52666) * Avoid copying global style presets via the styles compatibility hook (#52640) * i18n: Make the tab labels of `ColorGradientSettingsDropdown` component translatable (#52669) * Rich Text/Footnotes: fix getRichTextValues for useInnerBlocksProps.save (#52682) * Rich Text/Footnotes: fix getRichTextValues for useInnerBlocksProps.save * Address feedback * Patterns: Remove `reusable` text from menu once rename hint has been dismissed (#52664) * Show uncategorized patterns on the Editor > Patterns page (#52633) * Patterns: fix bug with Create Patterns menu not showing in site editor page editing (#52671) * Pass the root client id into the reusable blocks menu * Check that clientIds array is defined * Make check for array item more specific * Search block: Enqueue view script through block.json (#52552) * Search block: Enqueue view script through block.json * Remove extra space * Use `_get_block_template_file` function and set $area variable. (#52708) * Use `_get_block_template_file` function and set $area variable. * Update packages/block-library/src/template-part/index.php Co-authored-by: Felix Arntz <[email protected]> --------- Co-authored-by: Felix Arntz <[email protected]> * Site Editor: Don't allow creating template part on the Patterns page for non-block themes (#52656) * Don't allow template part to be created on the Patterns page for non-block themes * Remove unnecessary theme directory name in E2E test * Change Delete page menu item to Move to trash. (#52641) * Use relative path internally to include packages in dependencies (#52712) * Spacing Sizes: Fix zero size (#52711) * DimensionsPanel: Fix unexpected value decoding/encoding (#52661) --------- Co-authored-by: Daniel Richards <[email protected]> Co-authored-by: Saxon Fletcher <[email protected]> Co-authored-by: Jerry Jones <[email protected]> Co-authored-by: Robert Anderson <[email protected]> Co-authored-by: Andrea Fercia <[email protected]> Co-authored-by: Rich Tabor <[email protected]> Co-authored-by: James Koster <[email protected]> Co-authored-by: Miguel Fonseca <[email protected]> Co-authored-by: Haz <[email protected]> Co-authored-by: George Mamadashvili <[email protected]> Co-authored-by: Aki Hamano <[email protected]> Co-authored-by: Ella <[email protected]> Co-authored-by: Glen Davies <[email protected]> Co-authored-by: Carolina Nymark <[email protected]> Co-authored-by: Petter Walbø Johnsgård <[email protected]> Co-authored-by: Jonny Harris <[email protected]> Co-authored-by: Felix Arntz <[email protected]> Co-authored-by: Ramon <[email protected]> Co-authored-by: Andrew Serong <[email protected]>
|
edit: As of mid July 2024, I've started working on this again. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
talldan
force-pushed
the
migrate/block-editor-insert-usage-to-preferences-store
branch
4 times, most recently
from
3d066cf
to
6eb84df
Compare
Update tests Make updateInsertUsage a proper action that can be unit tested Fix reusable block tests Update test Try fixing private actions with new store registration Update tests Add special handling for the insertUsage migration as it was performed later than others Remove unused function Add mark next change as expensive action to preferences store Update debounce function to handle a longer debounce for expensive changes Mark the insertUsage preference change as expensive Make expensive calls use a trailing edge debounce Fix duplicate keys in tests Improve trailing edge test Fix tests, and ensure options object is optional Make updateInsertUsage a private API Make markNextChangeAsExpensive a private API Update docs Update package-lock Opt-in preferences as a core module using private apis Do not unlock what is already unlocked Remove time property from INSERT_BLOCKS and REPLACE_BLOCKS action objects Rename `match` to `variation` Rename file to create-async-debouncer Add an extra param for defining the debounce of expensive requests Add a default value of `false` for the `isExpensive` option Make `__unstableGetInsertUsage` a private selector called `getInsertUsage`. Remove `__unstableGetInsertUsageForBlock` Only run the migration when needed Keep the `insertUsage` data at a maximum of 100 records Fix linting issues Update docs
talldan
force-pushed
the
migrate/block-editor-insert-usage-to-preferences-store
branch
from
8f3f287
to
152c653
Compare
|
The changes to debouncing caused some interesting issues in e2e tests. A couple of e2e tests use the preferences API to set up a test state ( Instead I've switched to It also looks like preferences state is only ever reset at the start of running an entire test suite, not before individual files or test cases, so there can be lingering preferences state. Surprisingly it doesn't cause that many issues. 🤔 |
| await editor.setPreferences( 'core/edit-site', { | ||
| welcomeGuide: false, | ||
| } ); |
There was a problem hiding this comment.
This test actually fails in trunk if you run it locally. It only passes in CI in trunk because other tests that run before it set welcomeGuide to false 😬
I'm not sure why it started failing on CI in this PR.
Part of #31965
What?
The block editor package maintains some block insertion data to aid the 'Most used blocks' feature in the inserter. It persists this data.
This PR migrates the data to the preferences store.
Why?
As outlined in #31965, the goal is to move persisted data out of our various stores to a centralized preferences store.
How?
This required adding the preferences store as a dependency of the block editor package.The logic in the block editor's
preferencesreducer is now moved over to a new actionupdateInsertUsagethat uses the preferences store. This new private action is dispatched in theinsertBlocksandreplaceBlocksactions.The local storage data is migrated across to live under the preferences store from the old preferences system.
The preferences store's API usually uses a
scopeparameter that's usually set to 'core/edit-post' or 'core/edit-site', to indicate which editor the data belongs to.insertUsagestats are shared across editors, so I've used the 'core' scope.Finally, this PR introduces a new private action in the preferences store,
markNextChangeAsExpensive, which can be dispatched before updating a preference to delay the regularity with which the persistence layer makes HTTP requests to update user meta. It means you can insert loads of blocks quickly, but user meta will only update after a minute has passed. Insert usage still backs up immediately to a local storage persistence layer, so there's lots of resilience.Testing Instructions
trunkopen the post editor