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

Framework: Handle and upgrade deprecated blocks #3665

Merged
merged 4 commits into from
Dec 7, 2017
Merged

Framework: Handle and upgrade deprecated blocks #3665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
06fe513
Framework: Handle and upgrade deprecated blocks
youknowriad Nov 27, 2017
3863e50
Framework: Rename "deprecatedVersions" to "deprecated"
youknowriad Nov 27, 2017
3f94460
Quote Block: Drop the blocks-quote-style-* styles in favor of a is-la…
youknowriad Nov 27, 2017
bd6f99c
Parser: Extract the deprecated versions handler to a separate function
youknowriad Nov 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 40 additions & 1 deletion blocks/api/parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* External dependencies
*/
import { parse as hpqParse } from 'hpq';
import { mapValues } from 'lodash';
import { mapValues, omit } from 'lodash';

/**
* Internal dependencies
Expand Down Expand Up @@ -129,6 +129,32 @@ export function getBlockAttributes( blockType, innerHTML, attributes ) {
return blockAttributes;
}

/**
* Attempt to parse the innerHTML using using a supplied `deprecated` definition.
*
* @param {?Object} blockType Block type
* @param {string} innerHTML Raw block content
* @param {?Object} attributes Known block attributes (from delimiters)
* @return {Object} Block attributes
*/
export function getAttributesFromDeprecatedVersion( blockType, innerHTML, attributes ) {
if ( ! blockType.deprecated ) {
return;
}

for ( let i = 0; i < blockType.deprecated.length; i++ ) {
const deprecatedBlockType = {
...omit( blockType, [ 'attributes', 'save', 'supports' ] ), // Parsing/Serialization properties
...blockType.deprecated[ i ],
};
const deprecatedBlockAttributes = getBlockAttributes( deprecatedBlockType, innerHTML, attributes );
const isValid = isValidBlock( innerHTML, deprecatedBlockType, deprecatedBlockAttributes );
if ( isValid ) {
return deprecatedBlockAttributes;
}
}
}

/**
* Creates a block with fallback to the unknown type handler.
*
Expand Down Expand Up @@ -181,6 +207,19 @@ 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 parse it using a supplied `deprecated` definition.
// This allows blocks to modify their attribute and markup structure without invalidating
// content written in previous formats.
if ( ! block.isValid ) {
const attributesParsedWithDeprecatedVersion = getAttributesFromDeprecatedVersion(
blockType, innerHTML, attributes
);
if ( attributesParsedWithDeprecatedVersion ) {
block.isValid = true;
block.attributes = attributesParsedWithDeprecatedVersion;
}
}

return block;
}
}
Expand Down
102 changes: 102 additions & 0 deletions blocks/api/test/parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
getBlockAttributes,
asType,
createBlockWithFallback,
getAttributesFromDeprecatedVersion,
default as parse,
} from '../parser';
import {
Expand All @@ -15,6 +16,15 @@ import {
setUnknownTypeHandlerName,
} from '../registration';

const expectFailingBlockValidation = () => {
Copy link
Member

Choose a reason for hiding this comment

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

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

gutenberg/blocks/api/test/validation.js

Lines 33 to 43 in 9bfd837

/* 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

Copy link
Member

Choose a reason for hiding this comment

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

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 👍

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

describe( 'block parser', () => {
const defaultBlockSettings = {
attributes: {
Expand Down Expand Up @@ -164,6 +174,62 @@ describe( 'block parser', () => {
} );
} );

describe( 'getAttributesFromDeprecatedVersion', () => {
it( 'should return undefined if the block has no deprecated versions', () => {
const attributes = getAttributesFromDeprecatedVersion(
defaultBlockSettings,
'<span class="wp-block-test-block">Bananas</span>',
{},
);
expect( attributes ).toBeUndefined();
} );

it( 'should return undefined if no valid deprecated version found', () => {
const attributes = getAttributesFromDeprecatedVersion(
{
name: 'core/test-block',
...defaultBlockSettings,
deprecated: [
{
save() {
return 'nothing';
},
},
],
},
'<span class="wp-block-test-block">Bananas</span>',
{},
);
expect( attributes ).toBeUndefined();
expectFailingBlockValidation();
} );

it( 'should return the attributes parsed by the deprecated version', () => {
const attributes = getAttributesFromDeprecatedVersion(
{
name: 'core/test-block',
...defaultBlockSettings,
save: ( props ) => <div>{ props.attributes.fruit }</div>,
deprecated: [
{
attributes: {
fruit: {
type: 'string',
source: 'text',
selector: 'span',
},
},
save: ( props ) => <span>{ props.attributes.fruit }</span>,
},
],
},
'<span class="wp-block-test-block">Bananas</span>',
{},
);
expect( attributes ).toEqual( { fruit: 'Bananas' } );
} );
} );

describe( 'createBlockWithFallback', () => {
it( 'should create the requested block if it exists', () => {
registerBlockType( 'core/test-block', defaultBlockSettings );
Expand Down Expand Up @@ -211,6 +277,42 @@ describe( 'block parser', () => {
const block = createBlockWithFallback( 'core/test-block', '' );
expect( block ).toBeUndefined();
} );

it( 'should fallback to an older version of the block if the current one is invalid', () => {
registerBlockType( 'core/test-block', {
...defaultBlockSettings,
attributes: {
fruit: {
type: 'string',
source: 'text',
selector: 'div',
},
},
save: ( { attributes } ) => <div>{ attributes.fruit }</div>,
deprecated: [
{
attributes: {
fruit: {
type: 'string',
source: 'text',
selector: 'span',
},
},
save: ( { attributes } ) => <span>{ attributes.fruit }</span>,
},
],
} );

const block = createBlockWithFallback(
'core/test-block',
'<span class="wp-block-test-block">Bananas</span>',
{ fruit: 'Bananas' }
);
expect( block.name ).toEqual( 'core/test-block' );
expect( block.attributes ).toEqual( { fruit: 'Bananas' } );
expect( block.isValid ).toBe( true );
expectFailingBlockValidation();
} );
} );

describe( 'parse()', () => {
Expand Down
2 changes: 1 addition & 1 deletion blocks/library/quote/editor.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
.wp-block-quote.blocks-quote-style-1 {
.wp-block-quote:not(.is-large) {
border-left: 4px solid $black;
padding-left: 1em;
}
91 changes: 63 additions & 28 deletions blocks/library/quote/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* External dependencies
*/
import { isString, get } from 'lodash';
import classnames from 'classnames';

/**
* WordPress dependencies
Expand All @@ -26,36 +27,38 @@ const fromEditableValue = value => value.map( ( subValue ) => ( {
children: subValue,
} ) );

const blockAttributes = {
value: {
type: 'array',
source: 'query',
selector: 'blockquote > p',
query: {
children: {
source: 'node',
},
},
default: [],
},
citation: {
type: 'array',
source: 'children',
selector: 'cite',
},
align: {
type: 'string',
},
style: {
type: 'number',
default: 1,
},
};

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

attributes: {
Copy link
Member

Choose a reason for hiding this comment

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

Why move this away?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

value: {
type: 'array',
source: 'query',
selector: 'blockquote > p',
query: {
children: {
source: 'node',
},
},
default: [],
},
citation: {
type: 'array',
source: 'children',
selector: 'footer',
},
align: {
type: 'string',
},
style: {
type: 'number',
default: 1,
},
},
attributes: blockAttributes,

transforms: {
from: [
Expand Down Expand Up @@ -152,6 +155,7 @@ registerBlockType( 'core/quote', {
edit( { attributes, setAttributes, focus, setFocus, mergeBlocks, className } ) {
const { align, value, citation, style } = attributes;
const focusedEditable = focus ? focus.editable || 'value' : null;
const containerClassname = classnames( className, style === 2 ? 'is-large' : '' );

return [
focus && (
Expand Down Expand Up @@ -181,7 +185,7 @@ registerBlockType( 'core/quote', {
),
<blockquote
key="quote"
className={ `${ className } blocks-quote-style-${ style }` }
className={ containerClassname }
>
<Editable
multiline="p"
Expand Down Expand Up @@ -220,16 +224,47 @@ registerBlockType( 'core/quote', {

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

We might want to sit on it a bit otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

style={ { textAlign: align ? align : null } }
>
{ value.map( ( paragraph, i ) => (
<p key={ i }>{ paragraph.children && paragraph.children.props.children }</p>
) ) }
{ citation && citation.length > 0 && (
<footer>{ citation }</footer>
<cite>{ citation }</cite>
) }
</blockquote>
);
},

deprecated: [
{
attributes: {
...blockAttributes,
citation: {
type: 'array',
source: 'children',
selector: 'footer',
},
},

save( { attributes } ) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@youknowriad youknowriad Nov 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

const { align, value, citation, style } = attributes;

return (
<blockquote
className={ `blocks-quote-style-${ style }` }
style={ { textAlign: align ? align : null } }
>
{ value.map( ( paragraph, i ) => (
<p key={ i }>{ paragraph.children && paragraph.children.props.children }</p>
) ) }
{ citation && citation.length > 0 && (
<footer>{ citation }</footer>
) }
</blockquote>
);
},
},
],
} );
2 changes: 1 addition & 1 deletion blocks/library/quote/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
font-style: normal;
}

&.blocks-quote-style-2 {
&.is-large {
padding: 0 1em;
p {
font-size: 24px;
Expand Down
2 changes: 1 addition & 1 deletion blocks/test/fixtures/core__quote__style-1.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
<!-- wp:core/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>
<blockquote class="wp-block-quote"><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><cite>Matt Mullenweg, 2017</cite></blockquote>
<!-- /wp:core/quote -->
2 changes: 1 addition & 1 deletion blocks/test/fixtures/core__quote__style-1.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,6 @@
],
"style": 1
},
"originalContent": "<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>"
"originalContent": "<blockquote class=\"wp-block-quote\"><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><cite>Matt Mullenweg, 2017</cite></blockquote>"
}
]
2 changes: 1 addition & 1 deletion blocks/test/fixtures/core__quote__style-1.parsed.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"style": "1"
},
"innerBlocks": [],
"innerHTML": "\n<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>\n"
"innerHTML": "\n<blockquote class=\"wp-block-quote\"><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><cite>Matt Mullenweg, 2017</cite></blockquote>\n"
},
{
"attrs": {},
Expand Down
6 changes: 2 additions & 4 deletions blocks/test/fixtures/core__quote__style-1.serialized.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
<!-- wp:quote -->
<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>
<blockquote class="wp-block-quote">
<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><cite>Matt Mullenweg, 2017</cite></blockquote>
<!-- /wp:quote -->
2 changes: 1 addition & 1 deletion blocks/test/fixtures/core__quote__style-2.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
<!-- wp:core/quote {"style":"2"} -->
<blockquote class="wp-block-quote blocks-quote-style-2"><p>There is no greater agony than bearing an untold story inside you.</p><footer>Maya Angelou</footer></blockquote>
<blockquote class="wp-block-quote is-large"><p>There is no greater agony than bearing an untold story inside you.</p><cite>Maya Angelou</cite></blockquote>
<!-- /wp:core/quote -->
2 changes: 1 addition & 1 deletion blocks/test/fixtures/core__quote__style-2.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,6 @@
],
"style": 2
},
"originalContent": "<blockquote class=\"wp-block-quote blocks-quote-style-2\"><p>There is no greater agony than bearing an untold story inside you.</p><footer>Maya Angelou</footer></blockquote>"
"originalContent": "<blockquote class=\"wp-block-quote is-large\"><p>There is no greater agony than bearing an untold story inside you.</p><cite>Maya Angelou</cite></blockquote>"
}
]
2 changes: 1 addition & 1 deletion blocks/test/fixtures/core__quote__style-2.parsed.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"style": "2"
},
"innerBlocks": [],
"innerHTML": "\n<blockquote class=\"wp-block-quote blocks-quote-style-2\"><p>There is no greater agony than bearing an untold story inside you.</p><footer>Maya Angelou</footer></blockquote>\n"
"innerHTML": "\n<blockquote class=\"wp-block-quote is-large\"><p>There is no greater agony than bearing an untold story inside you.</p><cite>Maya Angelou</cite></blockquote>\n"
},
{
"attrs": {},
Expand Down
Loading