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

[WIP] Add a basic test for shortcode transformation #7689

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
31 changes: 20 additions & 11 deletions blocks/api/factory.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
every,
reduce,
castArray,
find,
findIndex,
includes,
isObjectLike,
Expand All @@ -27,15 +28,18 @@ import { getBlockType, getBlockTypes } from './registration';
/**
* Returns a block object given its type and attributes.
*
* @param {string} name Block name.
* @param {Object} blockAttributes Block attributes.
* @param {?Array} innerBlocks Nested blocks.
* @param {string} name Block name.
* @param {Object} blockAttributes Block attributes.
* @param {?Array} innerBlocks Nested blocks.
* @param {?Object} blockType Block type definition.
*
* @return {Object} Block object.
*/
export function createBlock( name, blockAttributes = {}, innerBlocks = [] ) {
export function createBlock( name, blockAttributes = {}, innerBlocks = [], blockType = undefined ) {
// Get the type definition associated with a registered block.
const blockType = getBlockType( name );
if ( undefined === blockType ) {
blockType = getBlockType( name );
}

// Ensure attributes contains only values defined by block type, and merge
// default values for missing attributes.
Expand Down Expand Up @@ -204,22 +208,27 @@ export function findTransform( transforms, predicate ) {
* If no block name is provided, returns transforms for all blocks. A normal
* transform object includes `blockName` as a property.
*
* @param {string} direction Transform direction ("to", "from").
* @param {?string} blockName Optional block name.
* @param {string} direction Transform direction ("to", "from").
* @param {?string} blockName Optional block name.
* @param {?Array} blockTypes Block types to get transforms from.
*
* @return {Array} Block transforms for direction.
*/
export function getBlockTransforms( direction, blockName ) {
export function getBlockTransforms( direction, blockName, blockTypes ) {
if ( blockTypes === undefined ) {
blockTypes = getBlockTypes();
}
// When retrieving transforms for all block types, recurse into self.
if ( blockName === undefined ) {
return flatMap(
getBlockTypes(),
( { name } ) => getBlockTransforms( direction, name )
blockTypes,
( { name } ) => getBlockTransforms( direction, name, blockTypes )
);
}

// Validate that block type exists and has array of direction.
const { transforms } = getBlockType( blockName ) || {};
const blockType = find( blockTypes, { name: blockName } );
const { transforms } = blockType || {};
if ( ! transforms || ! Array.isArray( transforms[ direction ] ) ) {
return [];
}
Expand Down
4 changes: 2 additions & 2 deletions blocks/api/raw-handling/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import 'element-closest';
* Internal dependencies
*/
import { createBlock, getBlockTransforms, findTransform } from '../factory';
import { getBlockType } from '../registration';
import { getBlockType, getBlockTypes } from '../registration';
import { getBlockAttributes, parseWithGrammar } from '../parser';
import normaliseBlocks from './normalise-blocks';
import specialCommentConverter from './special-comment-converter';
Expand Down Expand Up @@ -126,7 +126,7 @@ export default function rawHandler( { HTML = '', plainText = '', mode = 'AUTO',

// An array of HTML strings and block objects. The blocks replace matched
// shortcodes.
const pieces = shortcodeConverter( HTML );
const pieces = shortcodeConverter( HTML, 0, getBlockTransforms( 'from' ), getBlockTypes() );

// The call to shortcodeConverter will always return more than one element
// if shortcodes are matched. The reason is when shortcodes are matched
Expand Down
19 changes: 9 additions & 10 deletions blocks/api/raw-handling/shortcode-converter.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { some, castArray, first, mapValues, pickBy, includes } from 'lodash';
import { castArray, find, first, includes, mapValues, pickBy, some } from 'lodash';

/**
* WordPress dependencies
Expand All @@ -11,14 +11,10 @@ import { regexp, next } from '@wordpress/shortcode';
/**
* Internal dependencies
*/
import { createBlock, getBlockTransforms, findTransform } from '../factory';
import { getBlockType } from '../registration';
import { createBlock, findTransform } from '../factory';
import { getBlockAttributes } from '../parser';

function segmentHTMLToShortcodeBlock( HTML, lastIndex = 0 ) {
// Get all matches.
const transformsFrom = getBlockTransforms( 'from' );

function segmentHTMLToShortcodeBlock( HTML, lastIndex = 0, transformsFrom = [], blockTypes = [] ) {
const transformation = findTransform( transformsFrom, ( transform ) => (
transform.type === 'shortcode' &&
some( castArray( transform.tag ), ( tag ) => regexp( tag ).test( HTML ) )
Expand All @@ -31,7 +27,7 @@ function segmentHTMLToShortcodeBlock( HTML, lastIndex = 0 ) {
const transformTags = castArray( transformation.tag );
const transformTag = first( transformTags );

let match;
let blockType, match;

if ( ( match = next( transformTag, HTML, lastIndex ) ) ) {
const beforeHTML = HTML.substr( 0, match.index );
Expand All @@ -58,16 +54,19 @@ function segmentHTMLToShortcodeBlock( HTML, lastIndex = 0 ) {
( schema ) => schema.shortcode( match.shortcode.attrs, match ),
);

blockType = find( blockTypes, { name: transformation.blockName } );
Copy link
Member

Choose a reason for hiding this comment

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

Noting that getBlockType is O(1) (object lookup by property) and this is O(n)

const block = createBlock(
transformation.blockName,
getBlockAttributes(
{
...getBlockType( transformation.blockName ),
...blockType,
attributes: transformation.attributes,
},
match.shortcode.content,
attributes,
)
),
[],
blockType
);

return [
Expand Down
91 changes: 91 additions & 0 deletions blocks/api/raw-handling/test/shortcode-converter.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,91 @@
/**
* External dependencies
*/
import { equal } from 'assert';

/**
* Internal dependencies
*/
import segmentHTMLToShortcodeBlock from '../shortcode-converter';
import {
getBlockTransforms,
} from '../../factory';
import {
getPhrasingContentSchema,
} from '../utils';

describe( 'segmentHTMLToShortcodeBlock', () => {
const blockTypes = [
{
name: 'core/paragraph',
transforms: {
from: [
{
type: 'raw',
// Paragraph is a fallback and should be matched last.
priority: 20,
selector: 'p',
schema: {
p: {
children: getPhrasingContentSchema(),
},
},
},
],
},
},
{
name: 'core/shortcode',
transforms: {
from: [
{
type: 'shortcode',
// Per "Shortcode names should be all lowercase and use all
// letters, but numbers and underscores should work fine too.
// Be wary of using hyphens (dashes), you'll be better off not
// using them." in https://codex.wordpress.org/Shortcode_API
// Require that the first character be a letter. This notably
// prevents footnote markings ([1]) from being caught as
// shortcodes.
tag: '[a-z][a-z0-9_-]*',
attributes: {
text: {
type: 'string',
shortcode: ( attrs, { content } ) => {
return content;
},
},
},
priority: 20,
},
],
},
},
];
const transformsFrom = getBlockTransforms( 'from', undefined, blockTypes );

it( 'should convert a standalone shortcode between two paragraphs', () => {
const original = `<p>Foo</p>

[foo bar="apple"]

<p>Bar</p>`;
const expected = [
`<p>Foo</p>

`,
{
attributes: {},
innerBlocks: [],
isValid: true,
name: 'core/shortcode',
uuid: 'invalid',
Copy link
Member Author

Choose a reason for hiding this comment

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

uuid being a random value is the cause of the test failure right now. What's the best way of addressing?

Copy link
Member

Choose a reason for hiding this comment

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

Not entirely sure it works in this specific usage, but perhaps expect.any( String ) ?

http://jestjs.io/docs/en/expect#expectanyconstructor

Copy link
Member Author

Choose a reason for hiding this comment

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

Not entirely sure it works in this specific usage

Yeah, I suppose I could make the assertions a bit more verbose and inspect each part of the array.

Another idea is to create a wrapper for the uuid function that lets us inject some state for the purposes of testing.

Copy link
Member

Choose a reason for hiding this comment

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

expect.any( String ) is better, we don't care about what uuid produces in this context.

},
`

<p>Bar</p>`,
];
const transformed = segmentHTMLToShortcodeBlock( original, 0, transformsFrom, blockTypes );
equal( transformed, expected );
} );
} );