Editor: Make sure createInnerBlockList never updates when passed using context #6420
Conversation
gziolo
added
[Type] Enhancement
| @@ -96,10 +102,7 @@ export class BlockListBlock extends Component { | |||
| // is defined in `@wordpress/blocks`, so to avoid a circular dependency | |||
| // we inject this function via context. | |||
| return { | |||
| createInnerBlockList: ( uid ) => { | |||
There was a problem hiding this comment.
I think it may have been expected that getChildContext wasn't called as frequently as it actually is. While this works, an alternative may be to simply update this to the React 16.3.0 syntax. In either case, we should want the function to point to a consistent reference (the legacy API may actually make this easier for object context).
There was a problem hiding this comment.
Playing with when "context creators" are called: https://codepen.io/aduth/pen/LmZgKN?editors=1111
There was a problem hiding this comment.
https://codepen.io/anon/pen/vjXXOa?editors=1111 - when you add similar console.log calls for the new context, there is no difference in the way they re-render. There might be something I'm missing in here.
Anyways, I think that in the long createInnerBlockList could be used directly as a dependency without using context at all. We would have to move components from wp.blocks to wp.editor and apply a few more refactorings.
Should we apply this PR as a short-term solution and explore optimizing modules structure to remove obsolete usage of context?
There was a problem hiding this comment.
Anyways, I think that in the long createInnerBlockList could be used directly as a dependency without using context at all.
I'm curious how you'd see this working. Do you mean being explicit with how a block must pass its own rootUID? The existence of this context is largely to simplify the implementation behavior for block authors (similar in many ways to the auto-handling of isSelected).
There was a problem hiding this comment.
It should be easier to reason after Framework: Move components from the blocks to the editor module (#6521) gets merged. It seems like you would be able to use createInnerBlockList as an internal function, but you would still have to pass down renderBlockMenu. I would investigate once all blocks components are moved to editor.
blocks/block-edit/index.js
Outdated
| @@ -50,7 +52,26 @@ export class BlockEdit extends Component { | |||
| }; | |||
| } | |||
|
|
|||
| componentDidUpdate( prevProps, prevState ) { | |||
| const notEqual = ! isShallowEqual( prevProps, this.props ); | |||
| console.log( 'BlockEdit did update:', notEqual, ! isShallowEqual( prevState, this.state ) ); | |||
There was a problem hiding this comment.
I'm assuming we should drop these debugging changes (i.e. all of the changes to this file)?
There was a problem hiding this comment.
Yes, I have a separate commit to remove from this PR with debug only.
| @@ -96,10 +102,7 @@ export class BlockListBlock extends Component { | |||
| // is defined in `@wordpress/blocks`, so to avoid a circular dependency | |||
| // we inject this function via context. | |||
| return { | |||
| createInnerBlockList: ( uid ) => { | |||
There was a problem hiding this comment.
Anyways, I think that in the long createInnerBlockList could be used directly as a dependency without using context at all.
I'm curious how you'd see this working. Do you mean being explicit with how a block must pass its own rootUID? The existence of this context is largely to simplify the implementation behavior for block authors (similar in many ways to the auto-handling of isSelected).
gziolo
force-pushed
the
fix/crate-inner-block-list
branch
from
b64b5d5
to
734f28a
Compare
Debug changes were removed. |
Description
When working on another PR I discovered that
createInnerBlockListpassed through context toBlockEdittrigger tons of rerenders. This PR tries to fix it.I included also
shouldComponentUpdate( nextProps, nextState )method insideBlockEditcomponent in 909844f commit. It is bundled with debugging tools by design, because it should be fixed with another PR which @youknowriad is working on: #6313. In effect, this PR depends on #6313 and shouldn't be merged before we confirm thatpureHOC resolves all re-renders whenpropsandstatedon't change.How has this been tested?
In the browser using some debugging hacks on the JS console. I included them in the PR with
909844f. It must be removed before this branch gets merged :)
In general, we need to make sure that there is no regression introduced in all blocks, in particular, nested blocks should be examined very carefully.
Screenshots
Before
After
Types of changes
Performance improvements.
Checklist: