core-data: remove dependency on block-editor #67870
Conversation
jsnajdr
added
[Type] Code Quality
jsnajdr
requested review from
Mamaduka,
youknowriad,
gziolo,
ellatrix and
tyxla
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: -25 B (0%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for working on it @jsnajdr!
I might be approaching this one naively, but I'm not convinced this completely solves the pre-existing separation of concerns issue.
I still feel it's weird that this entire footnotes code belongs to the core-data package. I think it could be a good opportunity to add some extensibility to it and hook the footnotes code from outside. In other words, updateFootnotesFromMeta shouldn't be part of the core-data package IMHO, and that on its own should allow decoupling the block-editor package from core-data and solve the issue we're solving in this PR.
Additionally, any rich text handling utilities in my head should belong to the @wordpress/rich-text package. I find it odd that we're now moving parts of that to @wordpress/blocks and making things even more entangled. Why should @wordpress/blocks know about RichTextContent?
I realize some of the limitations are pre-existing, but if we're moving things around, I'd expect we take a step back and rethink where everything should really live.
Regardless of the direction, I agree the core-data package shouldn't depend on the block-editor package, and I appreciate the work done to solve that 👏
|
I personally like the approach in this PR. As I said,
Why? |
No particularly strong feelings, especially because I'm not as familiar with the code as some of you - I'm happy to disagree and commit.
Because Footnotes is just another block. I expect any custom features related to storing, retrieving, parsing, or updating that block to be a responsibility of that block, not of the module that works with core entities. To me, the fact we need to keep this custom block attribute handling in If it helps, try to imagine Footnotes didn't exist, and you were a plugin author, and you wanted to build a Footnotes block - how would you approach that? What extensibility points would you be missing? |
It seems this is the crux of the disagreement. You see "Footnotes" as just another block. For me it's really not. It's a property of a "post" just like "title", "content", or "excerpt". I think if you consider it this way, it makes more sense for core-data to own this. |
Right, and I still disagree with that, because I don't really see Footnotes as a property of the post, not the same way as I look at title, content, or excerpt anyway. Sorry, but your analogy just doesn't make sense to me. Title, content, and excerpt are true artifacts of the post, literally stored in the posts table. Footnotes is a byproduct of the content, and the fact we store it in meta and need to keep them in sync is just an implementation detail. Then again, Footnotes is still yet another block, and one that a plugin can and should be able to deregister. By keeping some of its internals in Also, a 3rd party plugin could also want to sync some of its content with post meta fields, and they should be able to do that through extending the core experience. I don't see how that would make it relevant for |
I wonder if the ToC block is a good candidate for that. Pparse heading blocks and sync them to ToC attributes. |
Yes, it could be a feature plugin that doesn't need to be hardwired in the core editor. And we are not very far from that, most of the feature is already implemented using extensibility APIs:
A real-world plugin would typically patch the select( 'core' ).editEntityRecord = wrap( originalEditEntityRecord );and achieve this goal even without a dedicated filter. If we have multiple use cases for such a hook, maybe we will eventually get there.
Because a major part of Also, a parsed Hardwired |
This PR removes the
block-editordependency from thecore-datapackage, by implementing thegetBlockFootnotesOrderfunction only using theelement,blocksandrich-textpackages. It implements the plan I laid out in #67687 (comment). Fixes #64604.I'm removing the
getRichTextValuesprivate export fromblock-editorand adding four new exports toblocks:getRichTextValues: traverses a tree of parsed blocks and extracts allRichTextDatavalues from them. It uses the "serializer" code fromblocks,RichTextData.fromHTMLStringconstructor fromrich-text, and it also needs to know the identity of theInnerBlocks.ContentandRichText.Contentfunction components so that it can recognize them in the JSX markup returned by blocksave()functions.InnerBlocksContent: a simple component intended to be interpreted only by the block serializer. Used insave()functions to render inner blocks.RichTextContent: another component used bysave()functions to render rich-text attributes. A thin wrapper aroundvalueToHTMLString, serializingRichTextDatavalues into HTML strings.valueToHTMLString: a think wrapper aroundRichTextData.toHTMLString, with some extra code that handles deprecated stuff (valueas array,multilineprop). This should probably be inrich-textinstead ofblocks. Or maybe we could remove it completely and callvalue.toHTMLStringdirectly?Open questions are:
valueToHTMLStringbe moved torich-text? Or can we remove it completely? It's used byRichText.Contentwhen serializing (saving) blocks and also byblock-editorRichTextcomponent in preview mode (see also Preview: skip rendering rich text #60544).