Refactor reusable blocks and editor effects to action-generators using controls (core/editor data store)

[nerrad]

Description

This completes the refactor of effects to action-generator for the core/editor package. The changes here are pretty straightforward and preserve the behaviour in the original effects.

How has this been tested?

• rebuilt unit test coverage and ensure passes
• Tested reusable blocks manually ensure there's no breakage.
• Ensure no e2e tests break.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.

General note

One thing I noticed when working on this refactor is that it's not immediately clear what the difference is between reusable block behaviour in the core/block-editor store/package and reusable block behaviour in the core/editor store/package. I realize that the core/block-editor package was created as a part of the ongoing work to make it possible to have standalone block editors apart from the post editor view. However, I didn't find any documentation for devs to know the distinction between the two. I suspect this is in part because this work is still in transition, but for future development it'd be useful to have something clarity outlined in the core/block-editor documentation about the state it maintains and what criteria is used for state maintained in it.

[nerrad]

Note, the above two dispatches (replaceBlocks and receiveBlocks) used to be _after the _experimentalSaveReusableBlock dispatch in the effects. However in testing after the conversion the reusable block conversion failed because the block wouldn't actually get replaced. I suspect this worked with effects because the timing worked a bit differently there. Switching these around works okay.

[nerrad]

@aduth since you reviewed the previous refactor in this package I included you for reviewers as well. This has the potential to go stale quickly so getting it merged in sooner would be ace.

[nerrad]

I'm investigating the e2e fails, on the surface they look legit.

[nerrad]

Pretty crazy, I'm reproducing the e2e fail. Selecting multiple blocks and then clicking to convert to reusable blocks completely locks up the view! No errors in the console, just inability to interact with the view. Closing the tab is not possible as well. Going to try and nail down what's happening.

[nerrad]

So it looks like its related to the switch in dispatch order I added.

[nerrad]

So here's what happens if I restore the dispatches:

	yield dispatch(
		'core/block-editor',
		'replaceBlocks',
		clientIds,
		createBlock( 'core/block', { ref: reusableBlock.id } )
	);

	// Re-add the original block to the store, since replaceBlock() will have removed it
	yield dispatch(
		'core/block-editor',
		'receiveBlocks',
		[ parsedBlock ]
	);

So they are after the __experimentalSaveReusableBlock dispatch.

The reusable block saves, but the subsequent __experimentalFetchReusableBlocks results in a request to /wp/v2/blocks/reusable2 (or whatever the temp id for the reusable block is), which of course fails because that is not the id it is saved as.

If I, put the actions before (as they are currently in this branch) then SINGLE blocks convert to reusable blocks with no issues, but multiple selected blocks cause the browser to crash.

I'll still investigate but just posting my findings so far because so far its stumping me.

[nerrad]

I think it may be related to withSaveReusableBlock in block-editor:

tate.attributes = mapValues( state.attributes, ( attributes, clientId ) => {
			const { name } = state.byClientId[ clientId ];
			if ( name === 'core/block' && attributes.ref === id ) {
				return {
					...attributes,
					ref: updatedId,
				};
			}

			return attributes;
		} );

At the time this is invoked (via the __experimentalSaveReusableBlock action in core/editor) the reusable block does not have a name of core/block, because it hasn't been replaced yet. So that means the attributes.ref for the block does not receive the updated id.

[aduth]

Noting: Is this perhaps in conflict with #14367, which would otherwise deprecate (or work toward deprecating) most of the dedicated reusable blocks handlers?

I'll plan to circle back to take a closer look.

[nerrad]

Noting: Is this perhaps in conflict with #14367, which would otherwise deprecate (or work toward deprecating) most of the dedicated reusable blocks handlers?

I think it's definitely related and I was semi aware of that pull. There's still not a whole lot removed I think from core/editor (but some modified) in that pull. One reason I tackled this is because I thought it might make things a bit clearer for doing that type of refactor. It does appear this pull is revealing some issues that will/are exposed with doing further refactoring maybe.

[aduth]

One thing I noticed when working on this refactor is that it's not immediately clear what the difference is between reusable block behaviour in the core/block-editor store/package and reusable block behaviour in the core/editor store/package.

Related: #13088 (comment)

I think ultimately neither should have too much awareness of how to deal with them. block-editor certainly seems it should have zero awareness of a reusable block in its dealing with rendering a parsed set of blocks. The editor module may need to have some awareness in the sense of preparing this parse arrangement, but ultimately I think it should be that:

• Reusable blocks are just posts. Treat them as such as far as data fetching / saving goes.
• Isolate the handling of reusable blocks to the implementation of core/block.

[aduth]

Is there any of this which can be extracted into smaller pull requests? setupEditor maybe? It's difficult to review pull requests of this size effectively.

[aduth]

It does appear this pull is revealing some issues that will/are exposed with doing further refactoring maybe.

It's made it clear to me that the refactor is far overdue, as the aggregate of technical debt in managing reusable blocks has made it exceedingly difficult to follow (particularly with the new fragmentation between editor and block-editor). I'm struggling to offer guidance without dedicating some hours to reeducating myself on the flow of reusable blocks; hours which personally I'd like to see spent toward having them be eliminated altogether.

Framed more productively, I wonder if we can't consider:

• Should this serve as an exploratory pull request, rather than one we might actually merge, to guide us in illuminating smaller tasks toward a refactor (whether that's Try refactoring the reusable blocks to use a separate block editor #14367, or some more intermediary steps)?
• Can those intermediary steps include: At least treating reusable blocks more like posts entities, even if we fear the task of Try refactoring the reusable blocks to use a separate block editor #14367 to be too large to achieve in the immediate future?
• Aside: It still seems setupEditor may be a separate task we could extract from this pull request.

I think it may also be worth getting some alignment on where and how we see "reusable blocks" existing as a thing in our implementations. I touched on this in #14491 (comment) , but in looking more at #14367 , I think I might not be seeing it quite the same as others. I'd have thought we should be able to eliminate almost all occurrences of the keyword "reusable" in the code of editor and block-editor. To me, the ideal implementation of core/block should be more-or-less edit: () => <Editor postId={ reusablePostId } /> (see also earlier #7453, example implementation). There shouldn't be any reusable-related actions, selectors, or reducer state. This contributes heavily to my considering of whether reimplementing the effects as action generators is worth the effort, depending whether we see it being a viable end-goal for them to not exist at all.

I'd like to revisit this tomorrow with a fresher mind.

[nerrad]

Should this serve as an exploratory pull request, rather than one we might actually merge, to guide us in illuminating smaller tasks toward a refactor (whether that's #14367, or some more intermediary steps)?

ya agreed.

Aside: It still seems setupEditor may be a separate task we could extract from this pull request.

Yup agreed I'll do in a separate pull.

Can those intermediary steps include: At least treating reusable blocks more like posts entities, even if we fear the task of #14367 to be too large to achieve in the immediate future?

Ya I would agree here as well. One thing that I noticed while doing this is that the block post type is repeatedly fetched via rest api in the __experimentalFetchReusableBlocks action (and elsewhere). That should probably be something stored in state (maybe via block-editor store?) to reduce the number of fetches.

To me, the ideal implementation of core/block should be more-or-less edit: () => <Editor postId={ reusablePostId } /> (see also earlier #7453, example implementation).

Hmm, ya that's interesting. I admittedly haven't dove to much into reusable blocks but it is apparent its in a very unstable state (likely due to evolution over time) and as referenced by all the __unstable and __experimental prefixes. Probably some of the difficulty lays in being able to convert to/from re-usable blocks maybe? Regardless, there definitely should be some focus on improving the api so I'll leave this pull open for reference but will split out the setupEditor into its own pull.

[nerrad]

setupEditor refactor moved to #14513

[nerrad]

I'm going to close this pull because it's quite out of date and I think refactors have been ongoing as code is changed. I refreshed #14594 which is ready to merge.