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.

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

Framework: Handle and upgrade deprecated blocks #3665

Merged
merged 4 commits into from Dec 7, 2017

Conversation

@youknowriad
Copy link
Contributor

commented Nov 27, 2017

refs #2541 alternative to #3327

Instead of adding a "version" to the block comment and upgrading blocks version by version, this uses a simpler alternative where:

  • A block adds keeps a list of deprecated versions ( save, attributes and support )
  • When parsing a block if it's invalid, we try to find a "valid" deprecated version
  • If we find a corresponding version, we use its definition to parse the block attributes.

Testing instructions

  • Paste this in to the code editor (old quote HTML)
<!-- wp:quote {"style":1} -->
<blockquote class="wp-block-quote blocks-quote-style-1">
    <p>The editor will endeavour to create a new page and post building experience that makes writing rich posts effortless, and has “blocks” to make it easy what today might take shortcodes, custom HTML, or “mystery meat” embed discovery.</p><footer>Matt Mullenweg, 2017</footer></blockquote>
<!-- /wp:quote -->
  • Click outside the editable area
  • You'll notice the block is "migrated" to the last version with cite and you can still edit the block as expected in the visual editor

@youknowriad youknowriad self-assigned this Nov 27, 2017

@youknowriad youknowriad requested review from mtias, aduth and dmsnell Nov 27, 2017

@codecov

This comment has been minimized.

Copy link

commented Nov 27, 2017

Codecov Report

Merging #3665 into master will decrease coverage by 0.27%.
The diff coverage is 77.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3665      +/-   ##
==========================================
- Coverage   37.74%   37.47%   -0.28%     
==========================================
  Files         279      277       -2     
  Lines        6743     6709      -34     
  Branches     1227     1219       -8     
==========================================
- Hits         2545     2514      -31     
  Misses       3536     3536              
+ Partials      662      659       -3
Impacted Files Coverage Δ
blocks/api/parser.js 98.41% <100%> (+0.41%) ⬆️
blocks/library/quote/index.js 18.6% <20%> (-3.28%) ⬇️
editor/components/warning/index.js 0% <0%> (-100%) ⬇️
components/panel/color.js 0% <0%> (-100%) ⬇️
components/panel/row.js 0% <0%> (-100%) ⬇️
blocks/autocompleters/index.js 0% <0%> (-42.11%) ⬇️
editor/utils/mobile/index.js 57.14% <0%> (-17.86%) ⬇️
editor/components/post-publish-button/label.js 83.33% <0%> (-4.17%) ⬇️
blocks/editable/format-toolbar/index.js 6.38% <0%> (-1.96%) ⬇️
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e60bd7...5a1ef20. Read the comment docs.

@@ -181,6 +181,26 @@ export function createBlockWithFallback( name, innerHTML, attributes ) {
// as invalid, or future serialization attempt results in an error
block.originalContent = innerHTML;

// If the block is invalid, try to find an older compatible version

This comment has been minimized.

Copy link
@mtias

mtias Nov 27, 2017

Contributor

Maybe expand a bit on this comment, as it's important to understand why.

Quick one:

When a block is invalid, attempt to validate again using a supplied `deprecated` definition.
This allows blocks to modify their attribute and markup structure without invalidating
content written in previous formats.
registerBlockType( 'core/quote', {
title: __( 'Quote' ),
icon: 'format-quote',
category: 'common',

attributes: {

This comment has been minimized.

Copy link
@mtias

mtias Nov 27, 2017

Contributor

Why move this away?

This comment has been minimized.

Copy link
@aduth

aduth Dec 4, 2017

Member

Why move this away?

To address this, it's because the deprecated and original versions share most of the same attributes, with deprecated assigning into to reflect the original attributes behavior of citation.

This comment has been minimized.

Copy link
@mtias

mtias Dec 7, 2017

Contributor

I have concerns with block looking different at first sight when you open one to learn from, that i think we should minimize.

) }
</blockquote>
);
},

deprecatedVersions: [

This comment has been minimized.

Copy link
@mtias

mtias Nov 27, 2017

Contributor

What about just deprecated here?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Nov 27, 2017

Author Contributor

Sounds good to me

deprecatedVersions: [
{
attributes: {
...blockAttributes,

This comment has been minimized.

Copy link
@mtias

mtias Nov 27, 2017

Contributor

I see. This is the kind of thing I think we should standardize. :)

},
},

save( { attributes } ) {

This comment has been minimized.

Copy link
@mtias

mtias Nov 27, 2017

Contributor

I have a slight concern with accumulating save mechanisms just for validation, but I don't see an alternative. Might be worth moving all the deprecated definitions into a deprecated.js file to not burden the main js with additional save definitions.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Nov 27, 2017

Author Contributor

What about the "attributes"? do you think we should share common attributes or maybe fine to duplicate?

@mtias

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2017

I like this approach a lot more.

@youknowriad youknowriad force-pushed the add/deprecated-blocks-handling branch from 39583ed to 37de716 Nov 27, 2017

@mtias

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2017

@youknowriad another side thing, I'd like to use this opportunity to change the class names used for the quote styles to be 'blocks-quote-style-1' = '' and 'blocks-quote-style-2' = 'is-large'.

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

commented Nov 27, 2017

@mtias Yep, sounds like a good idea. The quote here was just an example, thinking we should introduce these changes here or on follow-up PRs (with the several changes proposed by @samikeijonen )

@mtias

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2017

I'd be ok with introducing here if it serves a good testing purpose.

@@ -238,7 +240,7 @@ registerBlockType( 'core/quote', {

return (
<blockquote
className={ `blocks-quote-style-${ style }` }
className={ style === 2 ? 'is-large' : '' }

This comment has been minimized.

Copy link
@mtias

mtias Nov 27, 2017

Contributor

I think we could use default and large for the attribute itself? Or undefined and large maybe.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Nov 27, 2017

Author Contributor

mmm, this means the value of the attribute can change between two versions, I'm going to introduce a migrate function to handle this

This comment has been minimized.

Copy link
@mtias

mtias Nov 27, 2017

Contributor

Mmm, can it be handled in the attribute itself as a fallback?

This comment has been minimized.

Copy link
@mtias

mtias Nov 27, 2017

Contributor

We might want to sit on it a bit otherwise.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Nov 27, 2017

Author Contributor

No, we can't :(. Adding it is not so complex though 5dd81ce

This comment has been minimized.

Copy link
@mtias

mtias Nov 27, 2017

Contributor

Let's extract that commit into a separate PR so we can discuss the migration of values in isolation.

let attributesParsedWithDeprecatedVersion;
const hasValidOlderVersion = find( blockType.deprecated, ( oldBlockType ) => {
const deprecatedBlockType = {
...omit( blockType, [ 'attributes', 'save', 'supports' ] ), // Parsing/Serialization properties

This comment has been minimized.

Copy link
@aduth

aduth Nov 27, 2017

Member

Do we need to omit here, or is it enough that the spread will override those properties?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Nov 28, 2017

Author Contributor

Yes, we do because some of these properties are optional and we do not want to copy them in the old block's definition.

@@ -181,6 +181,28 @@ export function createBlockWithFallback( name, innerHTML, attributes ) {
// as invalid, or future serialization attempt results in an error
block.originalContent = innerHTML;

// When a block is invalid, attempt to validate again using a supplied `deprecated` definition.

This comment has been minimized.

Copy link
@aduth

aduth Nov 27, 2017

Member

Should we extract this logic to a separate function, or would it be possible to take advantage of recursion here in returning via createBlockWithFallback using the deprecated block attempt attributes in place of the original?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Nov 28, 2017

Author Contributor

Thanks for the suggestion Andrew, the code is better now. I preferred to avoid recursion because the logic is a bit simpler than createBlockWithFallback and it may include a specific "migrate" call in the future #3673

// content written in previous formats.
if ( ! block.isValid && blockType.deprecated ) {
let attributesParsedWithDeprecatedVersion;
const hasValidOlderVersion = find( blockType.deprecated, ( oldBlockType ) => {

This comment has been minimized.

Copy link
@aduth

aduth Nov 27, 2017

Member

Array#some (or Lodash's equivalent) would be a more appropriate method here, since we don't care about the found value, just that it exists.

};
const deprecatedBlockAttributes = getBlockAttributes( deprecatedBlockType, innerHTML, attributes );
const isValid = isValidBlock( innerHTML, deprecatedBlockType, deprecatedBlockAttributes );
attributesParsedWithDeprecatedVersion = isValid ? deprecatedBlockAttributes : undefined;

This comment has been minimized.

Copy link
@aduth

aduth Nov 27, 2017

Member

If we did extract this to a separate method, I could see this working well as an early return from a loop:

for ( let i = 0; i < deprecated.length; i++ ) {
	// ...
	if ( isValid ) {
		return deprecatedBlockAttributes;
	}
}

@youknowriad youknowriad force-pushed the add/deprecated-blocks-handling branch from 9240a66 to e6393ac Nov 28, 2017

@aduth
Copy link
Member

left a comment

Overall this looks good to me.

Two things I noted in testing:

  • The validation error for the original attempt shows even when there is a deprecated version available. This might be okay.
  • I added a {"style":2} block to post-content.js and it didn't upgrade to apply the is-large class name.

Try:

'<!-- wp:quote {"style":2} -->',
'<blockquote class="wp-block-quote blocks-quote-style-2"><p>The editor will endeavour to create a new page and post building experience that makes writing rich posts effortless, and has “blocks” to make it easy what today might take shortcodes, custom HTML, or “mystery meat” embed discovery.</p><footer>Matt Mullenweg, 2017</footer></blockquote>',
'<!-- /wp:quote -->',
@@ -15,6 +16,15 @@ import {
setUnknownTypeHandlerName,
} from '../registration';

const expectFailingBlockValidation = () => {

This comment has been minimized.

Copy link
@aduth

aduth Dec 4, 2017

Member

Aside: I had a similar need for validation tests, wondering if we ought to put the effort into a generalized "expect console errors" assertion:

/* eslint-disable no-console */
function expectError() {
expect( console.error ).toHaveBeenCalled();
console.error.mockClear();
}
function expectWarning() {
expect( console.warn ).toHaveBeenCalled();
console.warn.mockClear();
}
/* eslint-enable no-console */

Maybe expect( console ).toHaveLogged( 'error' ); set up in test/unit/setup-test-framework.js?

https://facebook.github.io/jest/docs/en/expect.html#expectextendmatchers

cc @gziolo

This comment has been minimized.

Copy link
@gziolo

gziolo Dec 6, 2017

Member

Nice idea! We are spying already on console warn and error methods: https://github.com/WordPress/gutenberg/blob/master/test/unit/setup-test-framework.js#L25. It should be easy to inplement 👍

selector: 'div',
},
},
save: ( { attributes } ) => <div>{attributes.fruit}</div>,

This comment has been minimized.

Copy link
@aduth

aduth Dec 4, 2017

Member

Hmm, we have this rule, but it is not capturing this:

https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-curly-spacing.md

We should seek to include whitespace inside the curly for these cases for consistency with similar rules.

registerBlockType( 'core/quote', {
title: __( 'Quote' ),
icon: 'format-quote',
category: 'common',

attributes: {

This comment has been minimized.

Copy link
@aduth

aduth Dec 4, 2017

Member

Why move this away?

To address this, it's because the deprecated and original versions share most of the same attributes, with deprecated assigning into to reflect the original attributes behavior of citation.

@youknowriad youknowriad force-pushed the add/deprecated-blocks-handling branch from e6393ac to 5a1ef20 Dec 6, 2017

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

commented Dec 6, 2017

I added a {"style":2} block to post-content.js and it didn't upgrade to apply the is-large class name.

It did upgrade for me with this exact same code, what am I missing here?

@mtias

This comment has been minimized.

Copy link
Contributor

commented Dec 7, 2017

The validation error for the original attempt shows even when there is a deprecated version available. This might be okay.

You mean the visual dialog prompting the user or in console?

@youknowriad youknowriad force-pushed the add/deprecated-blocks-handling branch from 5a1ef20 to bd6f99c Dec 7, 2017

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2017

I'm merging this one to include it in the release. It's working for me, can't reproduce the failing use-case

@youknowriad youknowriad merged commit 18a7b10 into master Dec 7, 2017

3 checks passed

codecov/project 37.74% (+0.01%) compared to da8dc5f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@aduth

This comment has been minimized.

Copy link
Member

commented Dec 11, 2017

I can't reproduce my originally reported issue on master. Maybe a fluke 👍

@aduth aduth referenced this pull request Dec 12, 2017
2 of 3 tasks complete

@gziolo gziolo deleted the add/deprecated-blocks-handling branch May 7, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.