Block Editor: Reimplement BlockEditorProvider using hooks #16357
Conversation
aduth
added
[Type] Performance
| onChange = () => {}, | ||
| onInput = () => {}, |
There was a problem hiding this comment.
Microoptimization: Instead of creating 2 separate functions these could both reference a dummy empty function.
There was a problem hiding this comment.
Specifically, the noop function provided by Lodash could be used here.
| const { blocks, isPersistent, isIgnored } = useSelect( ( select ) => { | ||
| const { | ||
| getBlocks, | ||
| isLastBlockChangePersistent, | ||
| __unstableIsLastBlockChangeIgnored, | ||
| } = registry.select( 'core/block-editor' ); | ||
| } = select( 'core/block-editor' ); | ||
|
|
||
| let blocks = getBlocks(); | ||
| let isPersistent = isLastBlockChangePersistent(); | ||
|
|
||
| this.unsubscribe = registry.subscribe( () => { | ||
| const { | ||
| onChange, | ||
| onInput, | ||
| } = this.props; | ||
| const newBlocks = getBlocks(); | ||
| const newIsPersistent = isLastBlockChangePersistent(); | ||
| if ( | ||
| newBlocks !== blocks && ( | ||
| this.isSyncingIncomingValue || | ||
| __unstableIsLastBlockChangeIgnored() | ||
| ) | ||
| ) { | ||
| this.isSyncingIncomingValue = false; | ||
| blocks = newBlocks; | ||
| isPersistent = newIsPersistent; | ||
| return; | ||
| return { | ||
| blocks: getBlocks(), | ||
| isPersistent: isLastBlockChangePersistent(), | ||
| isIgnored: __unstableIsLastBlockChangeIgnored(), | ||
| }; | ||
| } ); |
There was a problem hiding this comment.
A big optimization that could be made here is adding an empty dependency array to this useSelect.
| }; | ||
| } ); | ||
|
|
||
| const previousBlocks = useRef( blocks ); |
There was a problem hiding this comment.
This looks like a use-case for a usePrevious hook. I'm actually using one already in #21181, and I wonder if we should add it to @wordpress/compose.
There was a problem hiding this comment.
usePrevious has now been added to the @wordpress/compose package, so it could definitely be used here.
| import { useEffect, useRef } from '@wordpress/element'; | ||
| import { useSelect, useDispatch } from '@wordpress/data'; |
There was a problem hiding this comment.
Nitpick: I think imports should be in alphabetical order.
| import { useEffect, useRef } from '@wordpress/element'; | |
| import { useSelect, useDispatch } from '@wordpress/data'; | |
| import { useDispatch, useSelect } from '@wordpress/data'; | |
| import { useEffect, useRef } from '@wordpress/element'; |
|
I converted the block editor provider to use hooks in #21368. This PR could probably be changed to introduce any performance optimizations suggested? |
|
Thanks for pointing that out, @noahtallen. I've opened #23255 to add missing dependency arrays to Seeing as that the original purpose of this PR has already been fulfilled by another PR, I'm closing this one. Thanks for making this PR, @aduth, even if it didn't end up being used in the end. ❤️ |
This pull request seeks to refactor the
BlockEditorProvidercomponent to use React hooks in order to sync blocks value as effects leveraging existing data hooks for value subscriptions.The hope with this change was to simplify the logic flow, but I'd also hoped for some performance optimization to come from this simplification. However, it doesn't appear to have any benefit and in-fact has a minor negative impact (~106ms to 109ms average time to type via
npm run test-performance). It's unclear to me why this would be the case. While I've not dug into it, I'm curious ifuseSelectis as performance-optimized as it can be in cases where a component renders for reasons other than state changes (i.e. does it call the mapping selector even though state hasn't changed?).If the performance degradation cannot be resolved, I'm not sure whether it's worth moving forward with these changes.
If we do move forward with these changes, I think we could explore further flattening to reimplement the internal
withRegistryProviderhigher-order component as a hook.Testing Instructions:
Verify there are no regressions in standard usage (particularly via blocks updates, reusable blocks, etc).