-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add useSettings hook for reading multiple settings at once #55337
Conversation
@@ -126,48 +126,93 @@ export function shouldSkipSerialization( blockType, featureSet, feature ) { | |||
* @return {Object} Settings object. | |||
*/ | |||
export function useBlockSettings( name, parentLayout ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this particular function, my goal was to actually move its call to BlockEdit
or something and add it to the context of a block (probably as a private thing), that way it's only called once per block (as opposed to one per hook like today).
Size Change: +198 B (0%) Total Size: 1.66 MB
ℹ️ View Unchanged
|
Flaky tests detected in 64b12aee38f1f1fcb77f9015a8c1c91f66dfdeb6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6569868365
|
c029303
to
21134f9
Compare
I removed the commit with debugging code to make the CI check pass. You can manually apply it with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work 🥇
Thanks for diving deeper into the large-post typing performance issues. 👍
If useSettings
is preferred for multiple settings, I think we should ensure to educate folks about it (adding some additional explanation in docs) and maybe introduce an ESLint rule that prevents sibling useSetting()
calls?
@@ -125,7 +128,7 @@ export function setImmutably( object, path, value ) { | |||
* @return {*} Value of the object property at the specified path. | |||
*/ | |||
export const getValueFromObjectPath = ( object, path, defaultValue ) => { | |||
const normalizedPath = Array.isArray( path ) ? path : path.split( '.' ); | |||
const normalizedPath = Array.isArray( path ) ? path : stringToPath( path ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can totally be shipped as a separate PR regardless of how we decide to move forward.
* ```js | ||
* const isEnabled = useSetting( 'typography.dropCap' ); | ||
* ``` | ||
* @param {string[]} paths The path to the setting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @param {string[]} paths The path to the setting. | |
* @param {string[]} paths The paths to the settings. |
} | ||
return result; | ||
} | ||
|
||
/** | ||
* Hook that retrieves the given setting for the block instance in use. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Hook that retrieves the given setting for the block instance in use. | |
* Hook that retrieves the given settings for the block instance in use. |
} | ||
return result; | ||
} | ||
|
||
/** | ||
* Hook that retrieves the given setting for the block instance in use. | ||
* | ||
* It looks up the settings first in the block instance hierarchy. | ||
* If none is found, it'll look it up in the block editor store. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* If none is found, it'll look it up in the block editor store. | |
* If none are found, it'll look them up in the block editor store. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a commit that fixes all the typos in the docblocks.
*/ | ||
export default function useSetting( path ) { | ||
const { name: blockName, clientId } = useBlockEditContext(); | ||
export function useSettings( paths ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to move useSettings()
to a separate file? Perhaps under ../use-settings/index.js
? Right now it's a bit unfortunate that use-setting/index.js
exports useSettings()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hooks are very closely related, it's not justified to have them in separate files.
Most consumers should import them from the parent components
module, not using the /use-setting
subpath.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree because I still find it confusing and unexpected, but I'll agree to disagree and commit.
packages/block-editor/README.md
Outdated
Hook that retrieves the given setting for the block instance in use. | ||
|
||
It looks up the settings first in the block instance hierarchy. If none is found, it'll look it up in the block editor store. | ||
|
||
_Parameters_ | ||
|
||
- _paths_ `string[]`: The path to the setting. | ||
|
||
_Returns_ | ||
|
||
- `any[]`: Returns the values defined for the settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hook that retrieves the given setting for the block instance in use. | |
It looks up the settings first in the block instance hierarchy. If none is found, it'll look it up in the block editor store. | |
_Parameters_ | |
- _paths_ `string[]`: The path to the setting. | |
_Returns_ | |
- `any[]`: Returns the values defined for the settings. | |
Hook that retrieves the given settings for the block instance in use. | |
It looks up the settings first in the block instance hierarchy. If none are found, it'll look them up in the block editor store. | |
_Parameters_ | |
- _paths_ `string[]`: The paths to the settings. | |
_Returns_ | |
- `any[]`: Returns the values defined for the settings. |
|
||
function FontSizePicker( props ) { | ||
const fontSizes = useSetting( 'typography.fontSizes' ); | ||
const disableCustomFontSizes = ! useSetting( 'typography.customFontSize' ); | ||
const [ fontSizes, customFontSizes ] = useSettings( [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should keep the same name as the subsetting name:
const [ fontSizes, customFontSizes ] = useSettings( [ | |
const [ fontSizes, customFontSize ] = useSettings( [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I chose the variable name based on the name of the setting: customFontSize
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may be missing something, but that's exactly why I added the suggestion - in the PR we're using customFontSizes
and not customFontSize
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see 🙂 Changed as suggested.
|
||
return ( | ||
<BaseFontSizePicker | ||
{ ...props } | ||
fontSizes={ fontSizes } | ||
disableCustomFontSizes={ disableCustomFontSizes } | ||
disableCustomFontSizes={ ! customFontSizes } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
disableCustomFontSizes={ ! customFontSizes } | |
disableCustomFontSizes={ ! customFontSize } |
isTextEnabled, | ||
isHeadingEnabled, | ||
isButtonEnabled, | ||
] = useSettings( [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it could be nice to consider building an ESLint rule that prevents folks from having multiple useSetting
calls at the same level, recommending that we use useSettings()
instead.
Not for this PR, obviously, but something that we discussed earlier and I wanted to make sure it doesn't fall through the cracks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice update here, and one that is probably overdue. thanks for addressing this - the reduction in hook count is great.
export function useSettings( paths ) { | ||
const { name: blockName, clientId = null } = useBlockEditContext(); | ||
|
||
paths = useMemo( () => paths, paths ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the purpose of this useMemo
? am I mistaken in that it's doing nothing since all the internal function does is return its stored value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On each useSettings
call, the paths
array will be different, although it contains the same values. This useMemo
is to ensure that paths
is a constant in that case, and to ensure that useSelect
can memoize its return value. useSelect
itself does something similar internally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aha! the second value is the dependencies array, which in this case is paths
itself. in my head I was reading [paths]
mergeCache.set( value, result ); | ||
} | ||
return result; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be nice to have a docblock-style comment explaining the purpose of this function, and I wonder if the name could be clearer since mergeOrigins
doesn't outwardly communicate that there's a cache involved.
I'm also confused on the value we're storing here, or why we need flatMap()
. In the old code it's not clear to me either, so maybe this is why 🤷♂️ but it looks like it might be using .concat()
with a default of []
to avoid needing a conditional for the case where result
is missing the necessary key. I guess it's convenient for when keys are undefined and then it automatically returns an array with only those values.
but the old code was directly returning these things too while here we're storing them. since these are retrieved from value
is there a need to split them out at this point? did you find that extracting these three properties was showing up in the hot path?
I'm so confused on all of this, really. is it worth adding a separate function and cache vs. directly building the array every time? maybe it was all confused by the nature of the original use of reduce()
? I'm not trying to argue that the cache isn't worth it; I'm only asking if we have reason to believe it is.
const values = [];
[ 'default', 'theme', 'custom' ].forEach(
( key ) => {
if ( value[ key ] ) {
values.push( value[ key ] );
}
}
)
return values;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some settings, like color.palette
, have a "raw" value where settings from different "origins" are in separate arrays:
{
default: [ ... ],
theme: [ ... ],
custom: [ ... ],
}
The mergeOrigins
function merges the three arrays into one, concatenating them together. I added memoization so that two calls to useSelect
with the same state and arguments return the same value. This was a little pre-existing bug.
I added a docblock to the function that contains this explanation.
*/ | ||
export default function useSetting( path ) { | ||
const { name: blockName, clientId } = useBlockEditContext(); | ||
export function useSettings( paths ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you consider using variable argument counts here? it seems like most cases we'd be calling this we'll have literal values vs. dynamically-created arrays and I wonder if that's a convenience that would be nice.
const [ … ] = useSettings( 'setting-a', 'setting-b', 'color.setting.thing' );
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's a good idea. Variable arguments are better than an array argument. This is quite related to @tyxla's feedback about the module name:
Does it make sense to move
useSettings()
to a separate file? Perhaps under../use-settings/index.js
? Right now it's a bit unfortunate thatuse-setting/index.js
exportsuseSettings()
.
To solve both problems, we could do this:
- Rewrite
useSettings
to use variable arguments. - Migrate all remaining usages of
useSetting
touseSettings
. The entire difference is the return value as array:const [ a ] = useSettings( 'a' )
vsconst a = useSetting( 'a' )
. - This makes
useSetting
practically deprecated: there's no good reason any more to use it instead ofuseSettings
. - The actual source file can then be renamed to
use-settings/index.js
. The main thing it exports isuseSettings
, and then there's also the littleuseSetting
back-compat wrapper. I think that solves the naming concerns very well.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like that idea 👍 one fewer new API while still supporting all original use cases. Any good reason not to go that way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented! All usages of useSetting
are refactored to useSettings
, and the source folder name is now use-settings
.
c79b9f3
to
f337392
Compare
const usedLayout = ! layout?.type | ||
? { ...defaultLayout, ...layout, type: 'default' } | ||
: { ...defaultLayout, ...layout }; | ||
const { type = 'default' } = usedLayout; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tellthemachines This is a change that could be a separate PR. I found that the useSetting( 'layout' )
call can be completely removed, because the usedLayout
value is no longer used. Only its .type
field is used. And the type
can be calculated without knowing defaultLayout
. That value always comes from the layout
object.
usedLayout
was used until #47477 before usages of __experimentalLayout
, but today it's redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah well spotted! Yes, this can be removed. Not too fussed about whether it's done here or in a separate PR, your choice 😄
@jsnajdr a couple questions on deprecation, you may have addressed and I may have missed.
I wasn't sure if any imports of that being said, if we want to move away from |
I wasn't sure if I want to really formally deprecate
I think by now the names of the folders and files are sufficiently fine. The main thing that
This is just the internal layout of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great 🚀 Nice optimization!
I've left a few suggestions, but it's minor stuff and nothing really feels blocking.
I'm not particularly convinced that we should deprecate useSetting()
, I'd rather expect that we discourage sibling useSetting()
calls and encourage using useSettings()
in those cases, but this is something we could explore separately.
packages/block-editor/CHANGELOG.md
Outdated
@@ -2,21 +2,23 @@ | |||
|
|||
## Unreleased | |||
|
|||
- Deprecated the `useSetting` function in favor of new `useSettings` one that can retrieve multiple settings at once. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Deprecated the `useSetting` function in favor of new `useSettings` one that can retrieve multiple settings at once. | |
- Deprecated the `useSetting` function in favor of new `useSettings` one that can retrieve multiple settings at once ([#55337](https://github.com/WordPress/gutenberg/pull/55337)). |
@@ -944,9 +944,11 @@ _Parameters_ | |||
|
|||
### useSetting | |||
|
|||
> **Deprecated** | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add a note right here that it's deprecated in favor of useSettings()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note added 👍
@@ -98,7 +98,7 @@ function UncontrolledInnerBlocks( props ) { | |||
|
|||
const { allowSizingOnChildren = false } = defaultLayoutBlockSupport; | |||
|
|||
const defaultLayout = useSetting( 'layout' ) || EMPTY_OBJECT; | |||
const [ defaultLayout ] = useSettings( 'layout' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we keep the EMPTY_OBJECT
fallback here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not needed because defaultLayout
is used only with an object spread operator, ...defaultLayout
, which treats undefined
or null
as an empty object already.
|
||
const [ settingsSizes ] = useSettings( 'spacing.spacingSizes' ); | ||
if ( settingsSizes ) { | ||
spacingSizes.push( ...settingsSizes ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to avoid the mutation here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that local mutation is fine, it's not worse in any way than creating a new array and concatenating existing arrays into it with the spread operator.
|
||
// 0. Allow third parties to filter the block's settings at runtime. | ||
let result = applyFilters( | ||
'blockEditor.useSetting.before', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need to introduce a bulk settings filter right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly not, it would be counterproductive. If you want to modify a setting with a plugin, you want to register only one filter, not two of them, both useSetting
and useSettings
.
return storedResult; | ||
} | ||
|
||
describe( 'useSettings', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd keep a test for useSetting()
too, even if it's just a single one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test for useSetting
👍
@@ -68,14 +68,9 @@ function Controls( { | |||
[ minHeight ] | |||
); | |||
|
|||
const [ availableUnits ] = useSettings( 'spacing.units' ); | |||
const units = useCustomUnits( { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not from this PR, but this appears to be so repetitive! I wonder if we should abstract it to a separate hook with these default args.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! The default units change from case to case, but the rest is the same.
const layout = useSetting( 'layout' ); | ||
const [ fluidTypographySettings, layout ] = useSettings( | ||
'typography.fluid', | ||
'layout' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we get layout.wideSize
directly here since that's the only one we need?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, but I'll keep the existing 'layout'
retrieval. The codebase consistently uses 'layout'
, never its subfields.
const spacingSizes = [ { name: 0, slug: '0', size: 0 } ]; | ||
const [ settingsSizes ] = useSettings( 'spacing.spacingSizes' ); | ||
if ( settingsSizes ) { | ||
spacingSizes.push( ...settingsSizes ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice if we could avoid mutation.
* ``` | ||
*/ | ||
export function useSetting( path ) { | ||
deprecated( 'wp.blockEditor.useSetting', { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the same time, I'm not convinced useSetting()
really needs to be deprecated. It uses useSettings()
under the hood, so what difference does it make if we continue using it when we want to retrieve a single setting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retrieving a single setting is very ergonomic even with useSettings
. Having also useSetting
doesn't add any interesting improvement. Therefore, it's a good idea to support only one canonical API, and clearly mark the other one as "inferior".
64b12ae
to
8a312be
Compare
This PR delivered a significant improvement in performance tests, namely for the "Type" and "Inserter Hovering Items" stats. Immediately visible in the charts on codevitals.run: |
This should have been called out in the Misc Dev note for 6.4 as it deprecates a stabile hook |
I don't remember this being cherry-picked for 6.4. Based on the Gutenberg version this hasn't shipped with latest WP. |
Just added the dev note label to avoid forgetting about it for 6.5 |
I just downloaded WordPress 6.4.1 and inspected the |
Yes, 6.4 shipped with Gutenberg up to 16.7, so anything that's in 16.8 or newer and wasn't specifically cherry-picked didn't make it into the release. |
Thanks all for the confirmation and sorry for jumping to conclusions too early ❤️ Appreciate you all! |
Should we update the version number in the deprecation just for greater clarity? I created a PR for it here: #56377 |
@@ -112,6 +113,8 @@ export function setImmutably( object, path, value ) { | |||
return newObject; | |||
} | |||
|
|||
const stringToPath = memoize( ( path ) => path.split( '.' ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has the drawback of taking up memory that is never released. Maybe harmless here because it's small strings that probably get reused, but still worth nothing. I guess this is a weird case where a max size doesn't make sense. See also https://github.com/WordPress/gutenberg/pull/53406/files#r1286726012
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing in #56711. I remember when profiling, the memoization didn't have any impact on performance anyway.
_Usage_ | ||
|
||
```js | ||
const [ fixed, sticky ] = useSettings( 'position.fixed', 'position.sticky' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused here because the PR description says
const [ allowFixed, allowSticky ] = useSettings( [ 'position.fixed', 'position.sticky' ] );
Can both be used? It would be nice to be consistent so we don't need to normalise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the useSettings( 'a', 'b' )
form can be used. The PR originally started with an array argument, useSettings( settings: string[] )
, but then was changed to useSettings( ...settings: string[] )
during review. That's why the PR description is out of date.
The main reason for that was that when reading a single setting, useSetting( 'a' )
and useSettings( 'a' )
are the same, except that for useSettings
the return value is always an array (with one element).
Dev noteNew
|
Makes me wonder if we should just expose the |
Yes, of course, let's do that! After all, |
This is an attempt to improve the performance situation discussed in #54819, to make typing faster. I expected better results, but here it is anyway.
The problem is that 1000 blocks create 80k subscriptions to the
core/block-editor
store, and every action dispatch leads to calling 80k selectors and checking if their return value has changed. Of course, in a vast majority of cases the return value hasn't changed.useSelect
has async mode that's active on almost all of the 1000 blocks, but even adding an item to the priority queue 80k times takes some effort.This PR addresses one hook that is to blame for many (20k out of 80k) of the store subscriptions:
useSetting
.The first few commits perform optimizations on the existing
useSetting
hook, making it in total 40% faster. Don't create new strings, memoize splittinga.b.c
paths into['a', 'b', 'c']
arrays (lodash.get
always did this, too), etc.The second part of the PR is creating a new hook,
useSettings
. Instead of reading settings individually:we merge them into one hook call:
The point is that each
useSetting
hook has oneuseSelect
inside, leading to one store subscription per hook. My merging the setting reads together, we can save a lot of subscriptions.Migrating most usages of
useSetting
touseSettings
reduced the number ofblock-editor
subscription, on a large post with 1000 blocks, from 80k to 60k.The improvements are nice but still very incremental, they don't really solve the problem. We'll need to continue to search for other solutions.
The last commit contains some performance-measuring debug code I've been using. There are:
console.log
statements that report the number ofblock-editor
subscriptionsconsole.log
statements that report on how the async mode priority queue is being processed in a series ofrequestIdleCallback
s. Gives a good picture on how many store listeners we can process per second.I'll remove this debugging code before merging (hopefully 🙂)
This PR is best tested with a 4x CPU throttling enabled in the Performance tab in Chrome devtools.