Blocks: Stop showing errors and warnings when blocks gets successfully updated from deprecated version

[gziolo]

Description

This was something I wanted to tackle for quite some time. I decided to finally approach it when I worked on #16794 and I encountered this issue (#16794 (comment)):

When I go to Gutenberg demo page locally I see lots of warnings on JS console. I plan to open a related PR where I will improve the feedback shared on the JS console. It clearly is wrong as is, we should only let developers know that post was properly converted and show what was before and how it is coded from now on.

This is how the same demo page behaves when loaded with this branch:

Instead of throwing scary warnings and errors, the proposed implementation only informs on Web Console that block was successfully updated to the latest version.

How has this been tested?

Screenshots

Types of changes

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.

[gziolo]

@youknowriad and @mcsf - I'd love to hear your thoughts on the general direction proposed in this commit: 7be5763.

[youknowriad]

The end result is satisfying but I have some concerns about the "logger" implementation and the fact that we need a stateful object.

What if instead we just change the result of the validation functions from being a boolean with messaged logged to being an object containing a status and messages and let the caller decide what to do with these?

[gziolo]

What if instead we just change the result of the validation functions from being a boolean with messaged logged to being an object containing a status and messages and let the caller decide what to do with these?

I'm not happy about that as well. This was my biggest concern from the beginning but I wanted to focus on the experience first.

I considered something similar to what you described. The biggest concern is that isValidBlockContent is part of public API so we would have to find a way to make it backward compatible for those who expect boolean as a return value. Unless we add new options object param which would alow to change the return value:

/**
 * @param {string|Object} blockTypeOrName        Block type.
 * @param {Object}        attributes             Parsed block attributes.
 * @param {string}        originalBlockContent   Original block content.
 * @param {?Object}       options                Additional config options.
 * @param {?Boolean}      options.detailedReport Return all errors and warnings.
 *
 * @return {boolean|Object} Whether block is valid. When options.detailedReport
 *                          is set to true, this function will return an object with
 *                          the list of errors or warnings and isValid property.
 */
export function isValidBlockContent( blockTypeOrName, attributes, originalBlockContent, { detailedReport = false } ) {
    const isValid = true; // or false
    if ( detailedReport ) {
        return {
            isValid,
            logs,
        };
    }
    return isValid;
}

[youknowriad]

@gziolo We could use another function internally and make isValidBlockContent use that internal function and just returns the status and log the messages.

[mcsf]

Nice that you're working on this.

1. I agree with what's discussed around avoiding statefulness if practical.

2. The flow in the log can be a bit awkward depending on what is valid and invalid. Steps to repro:

2.1. Change the demo content by adding a bogus data attribute to the first DIV, like so:

diff --git a/post-content.php b/post-content.php
index 032c4ad10..7f3621ada 100644
--- a/post-content.php
+++ b/post-content.php
@@ -7,7 +7,7 @@
 
 ?>
 <!-- wp:cover {"url":"https://cldup.com/Fz-ASbo2s3.jpg","className":"alignwide"} -->
-<div class="wp-block-cover has-background-dim alignwide" style="background-image:url(https://cldup.com/Fz-ASbo2s3.jpg)"><div class="wp-block-cover__inner-container"><!-- wp:paragraph {"align":"center","placeholder":"Write title…","fontSize":"large"} -->
+<div class="wp-block-cover has-background-dim alignwide" data-foo="bar" style="background-image:url(https://cldup.com/Fz-ASbo2s3.jpg)"><div class="wp-block-cover__inner-container"><!-- wp:paragraph {"align":"center","placeholder":"Write title…","fontSize":"large"} -->
 <p style="text-align:center" class="has-large-font-size"><?php _e( 'Of Mountains &amp; Printing Presses', 'gutenberg' ); ?></p>
 <!-- /wp:paragraph --></div></div>
 <!-- /wp:cover -->

2.2. This will break the opening Cover block of the demo.

2.3. Open the demo post. Notice how the first line logged reads Block successfully updated for core/paragraph, which refers to the P tag inside the broken Cover block. The following lines are errors logged for the invalid Cover block as a whole.

2.4. The first line, about core/paragraph, should probably not be there, since in the end we didn't get a new Paragraph block in the block list, we only get an invalid Cover block.

[gziolo]

@gziolo We could use another function internally and make isValidBlockContent use that internal function and just returns the status and log the messages.

I followed this recommendation from @youknowriad and introduced an internal method getBlockContentValidationResult.

2.3. Open the demo post. Notice how the first line logged reads Block successfully updated for core/paragraph, which refers to the P tag inside the broken Cover block. The following lines are errors logged for the invalid Cover block as a whole.

2.4. The first line, about core/paragraph, should probably not be there, since in the end we didn't get a new Paragraph block in the block list, we only get an invalid Cover block.

I can reproduce that. Well, technically speaking it's correct because we validate/migrate nested blocks first. I understand this might be confusing. I guess, we could resolve that but I don't see it as a blocker since the proposed solution is way much better than what we have as of today. Anyway, I will look into a nice fix tomorrow :)

[mcsf]

Looks great! As you pointed out, any bits such as #16862 (comment) is something we can figure out later.

[senadir]

A recent discussion I saw raised this point, should those messages show up in production? at what stage should they be shown? for the end-user, only errors are relevant right?

[gziolo]

I don't have strong feelings, either on or off on production is fine :)

[mcsf]

A recent discussion I saw raised this point, should those messages show up in production? at what stage should they be shown? for the end-user, only errors are relevant right?

Since most users keep their console closed, what is the motivation for hiding the messages in production? Personally, I think they could be useful in assisting users experiencing issues.

[senadir]

well for now they show when nothing is wrong, might emit only error ones? ones that can be actionable?
as Greg said, I don't have stronger feelings as well, just raised a point I saw lately

[mcsf]

well for now they show when nothing is wrong, might emit only error ones? ones that can be actionable?

Reducing noise sounds good. :)