Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix initial block parsing #52417

Merged
merged 4 commits into from Jul 19, 2023
Merged

Fix initial block parsing #52417

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
275b0c6
Fix initial block parsing
youknowriad Jul 7, 2023
3cef5c9
Fix e2e test failures
youknowriad Jul 17, 2023
5513ddb
Update getEditorBlocks selector to retrieve initial blocks from content
youknowriad Jul 18, 2023
82dd82e
Fix unit tests
youknowriad Jul 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 12 additions & 20 deletions packages/core-data/src/entity-provider.js
View file
Open in desktop
Expand Up @@ -5,7 +5,7 @@ import {
createContext,
useContext,
useCallback,
useEffect,
useMemo,
} from '@wordpress/element';
import { useSelect, useDispatch } from '@wordpress/data';
import { parse, __unstableSerializeAndClean } from '@wordpress/blocks';
Expand Down Expand Up @@ -156,12 +156,12 @@ export function useEntityProp( kind, name, prop, _id ) {
export function useEntityBlockEditor( kind, name, { id: _id } = {} ) {
const providerId = useEntityId( kind, name );
const id = _id ?? providerId;
const { content, blocks, meta } = useSelect(
const { content, editedBlocks, meta } = useSelect(
( select ) => {
const { getEditedEntityRecord } = select( STORE_NAME );
const editedRecord = getEditedEntityRecord( kind, name, id );
return {
blocks: editedRecord.blocks,
editedBlocks: editedRecord.blocks,
content: editedRecord.content,
meta: editedRecord.meta,
};
Expand All @@ -171,23 +171,15 @@ export function useEntityBlockEditor( kind, name, { id: _id } = {} ) {
const { __unstableCreateUndoLevel, editEntityRecord } =
useDispatch( STORE_NAME );

useEffect( () => {
// Load the blocks from the content if not already in state
// Guard against other instances that might have
// set content to a function already or the blocks are already in state.
if ( content && typeof content !== 'function' && ! blocks ) {
const parsedContent = parse( content );
editEntityRecord(
kind,
name,
id,
{
blocks: parsedContent,
},
{ undoIgnore: true }
);
const blocks = useMemo( () => {
if ( editedBlocks ) {
return editedBlocks;
}
}, [ content ] );

return content && typeof content !== 'function'
? parse( content )
: EMPTY_ARRAY;
}, [ editedBlocks, content ] );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change has had one unpleasant side effect on the undo stack. When the post editor is initially loaded, the post entity in the Core Data store will have the content attribute, but there is no blocks attribute. The parsed blocks are stored only as a local memo state in this provider component, and there is no longer any editEntityRecord call that would save them in the store. The blocks attribute is set only on the first real edit.

The first real edit will produce an EDIT_ENTITY_RECORD action that has action.meta.undo.edits.blocks set to undefined. The state.undo reducer will create an undo stack record for this edit, and it will look like { property: 'blocks', from: undefined, to: [ ... ] }.

Applying that undo record means that the blocks attribute will be set to undefined, the value of the from field of the undo record. But instead of undefined, it should have been the original parsed blocks, the value is known.

Setting blocks to undefined means that on undo, they will need to be parsed from scratch from the content string and all client IDs will be different.

This will cause all blocks' edit components to be unmounted and re-created from scratch, on a simple undo operation that shouldn't do this. It should just update one little attribute of one little block.

We've seen the unpleasant effect of this when refactoring the TOC block in #54094 (comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting blocks to undefined means that on undo, they will need to be parsed from scratch from the content string and all client IDs will be different.
This will cause all blocks' edit components to be unmounted and re-created from scratch, on a simple undo operation that shouldn't do this. It should just update one little attribute of one little block.

I agree that ideally the components should be unmounted and recreated from scratch but making the "blocks" edit undefined on undo is (in isolation) a fine.

That said, I think maybe we need a way to tell core-data to load these transient values on initial "fetch" of the entities instead of leaving that to consumer code which will always lead to inconsistencies.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

making the "blocks" edit undefined on undo is (in isolation) a fine.

In this case it's not fine, because conceptually the blocks value is known and it's not undefined. It just lives outside the data store, in React state. Losing that value leads to losing information, namely the blocks' already assigned client IDs.

I don't know how to fix it, and the issue is not terribly urgent. I mainly wanted to document that it exists and write down what I found.


const updateFootnotes = useCallback(
( _blocks ) => {
Expand Down Expand Up @@ -356,5 +348,5 @@ export function useEntityBlockEditor( kind, name, { id: _id } = {} ) {
[ kind, name, id, updateFootnotes, editEntityRecord ]
);

return [ blocks ?? EMPTY_ARRAY, onInput, onChange ];
return [ blocks, onInput, onChange ];
}
25 changes: 13 additions & 12 deletions packages/editor/src/store/selectors.js
View file
Open in desktop
Expand Up @@ -10,6 +10,7 @@ import {
getFreeformContentHandlerName,
getDefaultBlockName,
__unstableSerializeAndClean,
parse,
} from '@wordpress/blocks';
import { isInTheFuture, getDate } from '@wordpress/date';
import { addQueryArgs, cleanForSlug } from '@wordpress/url';
Expand Down Expand Up @@ -42,15 +43,6 @@ import { getTemplatePartIcon } from '../utils/get-template-part-icon';
*/
const EMPTY_OBJECT = {};

/**
* Shared reference to an empty array for cases where it is important to avoid
* returning a new array reference on every invocation, as in a connected or
* other pure component which performs `shouldComponentUpdate` check on props.
* This should be used as a last resort, since the normalized data should be
* maintained by the reducer result in state.
*/
const EMPTY_ARRAY = [];

/**
* Returns true if any past editor history snapshots exist, or false otherwise.
*
Expand Down Expand Up @@ -1088,9 +1080,18 @@ export const isPublishSidebarEnabled = createRegistrySelector(
* @param {Object} state
* @return {Array} Block list.
*/
export function getEditorBlocks( state ) {
return getEditedPostAttribute( state, 'blocks' ) || EMPTY_ARRAY;
}
export const getEditorBlocks = createSelector(
( state ) => {
return (
getEditedPostAttribute( state, 'blocks' ) ||
parse( getEditedPostContent( state ) )
);
},
( state ) => [
getEditedPostAttribute( state, 'blocks' ),
getEditedPostContent( state ),
]
);

/**
* A block selection object.
Expand Down
10 changes: 10 additions & 0 deletions packages/editor/src/store/test/selectors.js
View file
Open in desktop
Expand Up @@ -2150,6 +2150,7 @@ describe( 'selectors', () => {
attributes: {
providerNameSlug: 'instagram',
},
innerBlocks: [],
},
],
},
Expand All @@ -2172,6 +2173,7 @@ describe( 'selectors', () => {
clientId: 567,
name: 'core/embed',
attributes: {},
innerBlocks: [],
},
],
},
Expand All @@ -2194,11 +2196,13 @@ describe( 'selectors', () => {
clientId: 123,
name: 'core/image',
attributes: {},
innerBlocks: [],
},
{
clientId: 456,
name: 'core/quote',
attributes: {},
innerBlocks: [],
},
],
},
Expand All @@ -2222,6 +2226,7 @@ describe( 'selectors', () => {
clientId: 123,
name: 'core/image',
attributes: {},
innerBlocks: [],
},
],
},
Expand All @@ -2245,6 +2250,7 @@ describe( 'selectors', () => {
clientId: 456,
name: 'core/quote',
attributes: {},
innerBlocks: [],
},
],
},
Expand All @@ -2270,6 +2276,7 @@ describe( 'selectors', () => {
attributes: {
providerNameSlug: 'youtube',
},
innerBlocks: [],
},
],
},
Expand All @@ -2295,6 +2302,7 @@ describe( 'selectors', () => {
attributes: {
providerNameSlug: 'soundcloud',
},
innerBlocks: [],
},
],
},
Expand All @@ -2318,11 +2326,13 @@ describe( 'selectors', () => {
clientId: 456,
name: 'core/quote',
attributes: {},
innerBlocks: [],
},
{
clientId: 789,
name: 'core/paragraph',
attributes: {},
innerBlocks: [],
},
],
},
Expand Down
10 changes: 8 additions & 2 deletions test/e2e/specs/editor/blocks/comments.spec.js
View file
Open in desktop
Expand Up @@ -313,8 +313,14 @@ test.describe( 'Post Comments', () => {
).toBeVisible();

// Check the block definition has changed.
const content = await editor.getEditedPostContent();
expect( content ).toBe( '<!-- wp:comments {"legacy":true} /-->' );
await expect.poll( editor.getBlocks ).toMatchObject( [
{
name: 'core/comments',
attributes: {
legacy: true,
},
},
] );

// Visit post
await page.goto( `/?p=${ postId }` );
Expand Down
14 changes: 11 additions & 3 deletions test/e2e/specs/editor/various/undo.spec.js
View file
Open in desktop
Expand Up @@ -174,8 +174,14 @@ test.describe( 'undo', () => {
await pageUtils.pressKeys( 'primary+a' );
await pageUtils.pressKeys( 'primary+b' );
await pageUtils.pressKeys( 'primary+z' );
const activeElementLocator = editor.canvas.locator( ':focus' );
await expect( activeElementLocator ).toHaveText( 'test' );
await expect.poll( editor.getBlocks ).toMatchObject( [
{
name: 'core/paragraph',
attributes: {
content: 'test',
},
},
] );
} );

test( 'Should undo/redo to expected level intervals', async ( {
Expand Down Expand Up @@ -353,7 +359,9 @@ test.describe( 'undo', () => {
// regression present was accurate, it would produce the correct
// content. The issue had manifested in the form of what was shown to
// the user since the blocks state failed to sync to block editor.
const activeElementLocator = editor.canvas.locator( ':focus' );
const activeElementLocator = editor.canvas.locator(
'[data-type="core/paragraph"] >> nth=0'
);
await expect( activeElementLocator ).toHaveText( 'original' );
} );

Expand Down