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

Editor: Defer block validation until after source application #16569

Closed
wants to merge 3 commits into from
Closed

Editor: Defer block validation until after source application #16569

wants to merge 3 commits into from

Conversation

aduth
Copy link
Member

@aduth aduth commented Jul 12, 2019

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:

npm run test-unit

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

@aduth aduth added [Feature] Block API API that allows to express the block paradigm. [Feature] Block Validation/Deprecation Handling block validation to determine accuracy and deprecation labels Jul 12, 2019
@aduth aduth requested a review from epiqueras July 12, 2019 19:02
This was referenced Jul 12, 2019
Closed
Closed
aduth
aduth commented Jul 12, 2019
View reviewed changes
packages/editor/src/store/actions.js
@@ -943,6 +959,10 @@ export function* resetEditorBlocks( blocks, options = {} ) {
yield* resetLastBlockSourceDependencies( Array.from( updatedSources ) );
}

if ( options.validate ) {
Copy link
Member Author

@aduth aduth Jul 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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 resetEditorBlocks with validate: 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.

epiqueras
epiqueras reviewed Jul 18, 2019
View reviewed changes
packages/blocks/src/api/parser.js Outdated
/**
* Options for parse.
*
* @property {Object<string,Function>} sources An object defining custom source
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this JSDoc matches. Might be a copy/paste thing?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

epiqueras
epiqueras reviewed Jul 18, 2019
View reviewed changes
packages/blocks/src/api/test/validation.js
@@ -662,4 +662,48 @@ describe( 'validation', () => {
expect( isValid ).toBe( true );
} );
} );

describe( 'validate', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also test for validation failures?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense.

epiqueras
epiqueras requested changes Jul 18, 2019
View reviewed changes
Copy link
Contributor

@epiqueras epiqueras left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple of comments on a JSDoc and the tests.

The E2E tests should also be fixed.

@aduth aduth force-pushed the update/defer-parse-validate-custom-sources branch from b16b364 to 44d7e5c Compare July 30, 2019 20:59
@aduth
Copy link
Member Author

aduth commented Jul 30, 2019

Looks good, just a couple of comments on a JSDoc and the tests.

The unit tests should be fixed now in the rebased 272e660.

The E2E tests should also be fixed.

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. page.waitFor( () => window.wp.data.select( 'core/editor' ).__unstableIsEditorReady() ) in createNewPost) were not fruitful.

@epiqueras
Copy link
Contributor

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. page.waitFor( () => window.wp.data.select( 'core/editor' ).__unstableIsEditorReady() ) in createNewPost) were not fruitful.

How should we proceed? Is it a symptom of an underlying issue or are the tests just not compatible?

@aduth
Editor: Run validation only for edited posts
dfa0d03 
A new post would not have original content against which to compare.
@aduth
Copy link
Member Author

aduth commented Jul 31, 2019

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 master because validation occurs during parse, and parse of a new post yields an empty array of blocks (the template syncs after the parse, bypassing validation).

@aduth
Copy link
Member Author

aduth commented Dec 11, 2019

As of #17153, meta attribute values are no longer initialized into parsed blocks, which invalidates this approach.

@aduth aduth closed this Dec 11, 2019
@aduth aduth deleted the update/defer-parse-validate-custom-sources branch December 11, 2019 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epiqueras epiqueras epiqueras requested changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@chrisvanpatten chrisvanpatten Awaiting requested review from chrisvanpatten

@ellatrix ellatrix Awaiting requested review from ellatrix

@gziolo gziolo Awaiting requested review from gziolo

@mkaz mkaz Awaiting requested review from mkaz

@talldan talldan Awaiting requested review from talldan

@youknowriad youknowriad Awaiting requested review from youknowriad

Assignees
No one assigned
Labels
[Feature] Block API API that allows to express the block paradigm. [Feature] Block Validation/Deprecation Handling block validation to determine accuracy and deprecation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Validation fails if attribute is persisted to post content & meta.
2 participants
@aduth @epiqueras