-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Refactor serializer: rearrange code for clarity and introspection #1148
Changes from 10 commits
1550d1c
1166e68
d495cf7
a05d749
24fdbe2
7e8c2c9
7d82605
f97ee6a
f97431f
05b15a0
065141e
470f1b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import { difference } from 'lodash'; | ||
import { isEmpty, map, reduce } from 'lodash'; | ||
import { html as beautifyHtml } from 'js-beautify'; | ||
|
||
/** | ||
|
@@ -36,35 +36,94 @@ export function getSaveContent( save, attributes ) { | |
return wp.element.renderToString( rawContent ); | ||
} | ||
|
||
const escapeDoubleQuotes = value => value.replace( /"/g, '\"' ); | ||
const replaceHyphens = value => value.replace( /-/g, '\\-' ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe it should be |
||
|
||
/** | ||
* Transform value for storage in block comment | ||
* | ||
* Some special characters and sequences should not | ||
* appear in a block comment header. This transformer | ||
* will guarantee that we store the data safely. | ||
* | ||
* @param {*} value attribute value to serialize | ||
* @returns {*} transformed value | ||
*/ | ||
const serializeValue = value => | ||
'string' === typeof value | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if it is an array or object that contains strings? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. they shouldn't be. we should already be JSON-serializing data if not a number or string. this will change if we stop using |
||
? replaceHyphens( escapeDoubleQuotes( value ) ) | ||
: value; | ||
|
||
/** | ||
* Returns comment attributes as serialized string, determined by subset of | ||
* difference between actual attributes of a block and those expected based | ||
* on its settings. | ||
* Returns attributes which ought to be saved | ||
* and serialized into the block comment header | ||
* | ||
* When a block exists in memory it contains as its attributes | ||
* both those which come from the block comment header _and_ | ||
* those which come from parsing the contents of the block. | ||
* | ||
* This function returns only those attributes which are | ||
* needed to persist and which cannot already be inferred | ||
* from the block content. | ||
* | ||
* @param {Object} realAttributes Actual block attributes | ||
* @param {Object} expectedAttributes Expected block attributes | ||
* @return {string} Comment attributes | ||
* @param {Object<String,*>} allAttributes Attributes from in-memory block data | ||
* @param {Object<String,*>} attributesFromContent Attributes which are inferred from block content | ||
* @returns {Object<String,*>} filtered set of attributes for minimum save/serialization | ||
*/ | ||
export function getCommentAttributes( realAttributes, expectedAttributes ) { | ||
// Find difference and build into object subset of attributes. | ||
const keys = difference( | ||
Object.keys( realAttributes ), | ||
Object.keys( expectedAttributes ) | ||
export function getCommentAttributes( allAttributes, attributesFromContent ) { | ||
// Iterate over attributes and produce the set to save | ||
return reduce( | ||
Object.keys( allAttributes ), | ||
( toSave, key ) => { | ||
const allValue = allAttributes[ key ]; | ||
const contentValue = attributesFromContent[ key ]; | ||
|
||
// save only if attribute if not inferred from the content and if valued | ||
return ! ( contentValue !== undefined || allValue === undefined ) | ||
? Object.assign( toSave, { [ key ]: allValue } ) | ||
: toSave; | ||
}, | ||
{}, | ||
); | ||
} | ||
|
||
// Serialize the comment attributes as `key="value"`. | ||
return keys.reduce( ( memo, key ) => { | ||
const value = realAttributes[ key ]; | ||
if ( undefined === value ) { | ||
return memo; | ||
} | ||
/** | ||
* Lodash iterator which transforms a key: value | ||
* pair into a string of `key="value"` | ||
* | ||
* @param {*} value value to be stringified | ||
* @param {String} key name of value | ||
* @returns {string} stringified equality pair | ||
*/ | ||
function asNameValuePair( value, key ) { | ||
return `${ key }="${ serializeValue( value ) }"`; | ||
} | ||
|
||
if ( 'string' === typeof value ) { | ||
return memo + `${ key }="${ value.replace( '"', '\"' ) }" `; | ||
} | ||
export function serializeBlock( block ) { | ||
const blockName = block.name; | ||
const blockType = getBlockType( blockName ); | ||
const saveContent = getSaveContent( blockType.save, block.attributes ); | ||
const saveAttributes = getCommentAttributes( block.attributes, parseBlockAttributes( saveContent, blockType ) ); | ||
|
||
const serializedAttributes = ! isEmpty( saveAttributes ) | ||
? map( saveAttributes, asNameValuePair ).join( ' ' ) + ' ' | ||
: ''; | ||
|
||
if ( ! saveContent ) { | ||
return `<!-- wp:${ blockName } ${ serializedAttributes }--><!-- /wp:${ blockName } -->`; | ||
} | ||
|
||
return ( | ||
`<!-- wp:${ blockName } ${ serializedAttributes }-->\n` + | ||
|
||
/** make more readable - @see https://github.com/WordPress/gutenberg/pull/663 */ | ||
beautifyHtml( saveContent, { | ||
indent_inner_html: true, | ||
wrap_line_length: 0, | ||
} ) + | ||
|
||
return memo + `${ key }="${ value }" `; | ||
}, '' ); | ||
`\n<!-- /wp:${ blockName } -->` | ||
); | ||
} | ||
|
||
/** | ||
|
@@ -74,30 +133,5 @@ export function getCommentAttributes( realAttributes, expectedAttributes ) { | |
* @return {String} The post content | ||
*/ | ||
export default function serialize( blocks ) { | ||
return blocks.reduce( ( memo, block ) => { | ||
const blockName = block.name; | ||
const blockType = getBlockType( blockName ); | ||
const saveContent = getSaveContent( blockType.save, block.attributes ); | ||
const beautifyOptions = { | ||
indent_inner_html: true, | ||
wrap_line_length: 0, | ||
}; | ||
const blockAttributes = getCommentAttributes( block.attributes, parseBlockAttributes( saveContent, blockType ) ); | ||
|
||
if ( ! saveContent ) { | ||
return memo + '<!-- wp:' + blockName + ' ' + blockAttributes + '/-->\n\n'; | ||
} | ||
|
||
return memo + ( | ||
'<!-- wp:' + | ||
blockName + | ||
' ' + | ||
blockAttributes + | ||
'-->' + | ||
'\n' + beautifyHtml( saveContent, beautifyOptions ) + '\n' + | ||
'<!-- /wp:' + | ||
blockName + | ||
' -->' | ||
) + '\n\n'; | ||
}, '' ); | ||
return blocks.map( serializeBlock ).join( '\n\n' ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this right? What if you have the string
"\\\\-"
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs to pair with the serializer to make sure we don't get here. we have have to face escaping in some manner. doing it here I think is the easiest way to accomplish this: "You must escape hyphens. You need not escape anything else."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, see #1088 for background on the issues with a double-hyphen in an HTML comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this means we should escape\
as well?