Editor: Defer block validation until after source application#16569
Editor: Defer block validation until after source application#16569aduth wants to merge 3 commits into
Conversation
aduth
added
[Feature] Block API
aduth
requested review from
ajitbohra,
chrisvanpatten,
ellatrix,
gziolo,
mkaz,
talldan and
youknowriad
as code owners
| yield* resetLastBlockSourceDependencies( Array.from( updatedSources ) ); | ||
| } | ||
|
|
||
| if ( options.validate ) { |
There was a problem hiding this comment.
I considered not making this an option and instead the default behavior. For standard editor usage, we always call resetEditorBlocks with validate: true. It only really makes sense as an option for usage where a third-party might dispatch this action with the result of a default wp.blocks.parse function call, and thus the re-validation would be redundant (note: it would be redundant, but not otherwise harmful aside the performance overhead).
For standard editor usage, we always call
resetEditorBlockswithvalidate: true.
Edit: In reflection, this statement is totally wrong, and the reason it's set up the way it is is so that we can call resetEditorBlocks without frequent validation, since we call it on e.g. every key press.
| /** | ||
| * Options for parse. | ||
| * | ||
| * @property {Object<string,Function>} sources An object defining custom source |
There was a problem hiding this comment.
I don't think this JSDoc matches. Might be a copy/paste thing?
There was a problem hiding this comment.
I don't think this JSDoc matches. Might be a copy/paste thing?
Oof. Good call. Updated to something more reasonable in the rebased 272e660.
| } ); | ||
| } ); | ||
|
|
||
| describe( 'validate', () => { |
There was a problem hiding this comment.
Should we also test for validation failures?
There was a problem hiding this comment.
Should we also test for validation failures?
I don't think it's necessary. The purpose of these test cases is not to verify validation (which are a separate set of test cases), but to ensure the assignment of the validation result.
epiqueras
left a comment
epiqueras
left a comment
There was a problem hiding this comment.
Looks good, just a couple of comments on a JSDoc and the tests.
The E2E tests should also be fixed.
aduth
force-pushed
the
update/defer-parse-validate-custom-sources
branch
from
b16b364 to
44d7e5c
Compare
The unit tests should be fixed now in the rebased 272e660.
These are trickier, and appear to be a legitimate issue. I thought perhaps it might have just been an issue of the editor "readiness" being deferred, but some rough attempts at debugging (e.g. |
How should we proceed? Is it a symptom of an underlying issue or are the tests just not compatible? |
A new post would not have original content against which to compare.
|
I suspect this may be improved / resolved by dfa0d03. Previously, we'd have issue with block templates where it would try to run validation over the template blocks, but because there is no original content against which to compare for a new post, the validation would error. This never occurs in |
|
As of #17153, meta attribute values are no longer initialized into parsed blocks, which invalidates this approach. |
2 participants
Fixes #4989
Related: #16402, #16316
This pull request seeks to opt out of validation for the initial blocks parse, instead awaiting the application of sourced attributes. This allows, for example, meta attribute values to be applied such that validation can occur with expected results (see #4989).
Testing Instructions:
Ensure unit tests pass:
Verify that updating a block with an attribute sourced by a meta attribute and persisted to the block's markup (via
save) is valid after saving and reloading the editor content.Example plugin: https://gist.github.com/aduth/e44d104996277dd9d6b08edf403030c5