Serializer: Adding a block state serializer (blocks => postContent) #348

youknowriad · 2017-03-29T11:51:47Z

In this PR, I'm creating a serializer to transform a block list to a raw post content string. The serialization is performed while switching between visual and text mode (and will be used for saving the post content)

With this PR, I'm hoping I can convince we should switch to just return a string from the save function. cc @aduth

mtias · 2017-03-30T09:57:23Z

blocks/serializer/index.js

+ * @return {String}        The post content
+ */
+export default function serialize( blocks ) {
+	return blocks.reduce( ( memo, block ) => {


This block is a bit hard to follow. It may be good to separate the attribute handling to separate functions, and possibly add some comments.

Yes, the difficulty here is to figure out which attributes needs to be serialized to the starting comment and which should be ignored (because they're already in the static content).

So what I'm doing is running the attributes object (or function) defined on each block just to know which "keys" (attributes) are extracted from the content.

mtias · 2017-03-30T10:03:11Z

blocks/serializer/index.js

+		const blockType = block.blockType;
+		const blockSettings = wp.blocks.getBlockSettings( blockType );
+		const rawContent = wp.element.renderToString(
+			blockSettings.save( block.attributes )


The save looks very odd out of context to me, it seems to imply an action instead of returning a value for the generated html.

I think we could rename it serialize and return the string directly in the function.
To avoid having to call renderToString and to avoid any extra React attributes. @aduth may disagree here?

The initial direction was decided primarily from the perspective of how it's used in implementing a block, since the duality between edit and save, both with a consistent return type, seemed the most direct way to communicate the intent for the implementer to create an incoming (editor) and outgoing (to save) description of the UI.

Alternative names to consider:

prepareForEdit, prepareForSave (not entirely accurate since it's not preparing, it's returning the element describing the UI)

renderEdit, renderSave

Honestly it doesn't really bother me that it looks awkward in the one internal snippet of code where we're actually invoking it. One change I'd like to make which could improve this is to treat save and edit as components, not mere functions, like started in #335:

https://github.com/WordPress/gutenberg/pull/335/files#diff-1b2cbb4a0a02cacc1be65d67ece68c21

return the string directly in the function.

If we upgrade to react@next (16 w/ Fiber), strings are valid return values of a component's (function's) render, so we could support string as an option without making any distinction.

I'm trying to be pragmatic here, we have several issues if we return an element (or a component) and I would like to move forward with this.

I think this resolves several issues:

Imagine using a component that requires lifecycle hooks, the rendering would be async.

How can we drop the data-react attributes easily

We can not return multiple tags, A text block can have multiple paragraphs

Currently the content parsed by the text element returns all the markup test, if we add a container element, we would change the markup even if we do not perform any change test.

The string return value resolves all these without tradeoffs IMO

Would output make sense?

youknowriad · 2017-03-30T10:20:18Z

I added some comments to explain the serializer algorithm.

Splitting the attributes serialization to its own method would have required duplicating some logic (computing the blockSettings and the rawContent) or passing those as parameters which creates a function with a weird API. I preferred keeping everything in the same function for now but with more comments.

youknowriad · 2017-03-30T11:23:43Z

Several things on the last commit:

I've flattened the blocks module folder structure (parser, serializer and registration file)
Extracting the registration to its own file solves the cycle dependency error
I fixed the tests not being launched but the unit test command
I added a unit test to the serializer.

The most controversial change here is renaming save to serialize and returning a string instead of an element. cc @aduth @mtias

I think this resolves several issues:

Rendering a string is synchronous, rendering an element could be async
No more React data attributes while serializing
We could return multiple tags or a single tags, while returning an element enforce the content of a block to be a single tag.

For a reason I can not find right now, using export * from './module' causes issues in our current setup.

aduth · 2017-03-30T12:58:15Z

blocks/README.md

-		return RandomImage( { category: attributes.category } );
+	serialize: function( attributes ) {
+		const src = 'http://lorempixel.com/400/200/' + props.category;
+		return `<img src="${ src }" alt="${ props.category }"/>`;


I can't help but this documentation only goes to highlight why this is an idea we should avoid:

What if category has a " in it?

Why can't we re-use RandomImage to generate the same markup?

What if my markup is more than just a single HTML element, like a gallery with potentially hundreds of elements?

What benefit does this ever have that even if your answer to any of the above is "just call renderToString", I as the developer must even come to know that renderToString is a thing?

aduth · 2017-03-30T13:00:24Z

element/index.js

+ * @param {wp.Element} element Element to render
+ * @return {String}            HTML
+ */
+export function renderToString( element ) {


We should use react-dom/server's renderToStaticMarkup which, unlike renderToString, includes no checksum or reactid.

aduth · 2017-03-30T13:01:49Z

blocks/serializer.js

+		const blockSettings = getBlockSettings( blockType );
+
+		// static content, to be rendered inside the block comment
+		const rawContent = blockSettings.serialize( block.attributes );


Can we be certain blockSettings is not undefined here? What about blocks which had been added previously from a plugin that since has been deactivated?

Good question. I assume the block parser will already fallback to the default block in this case. HTML block.

aduth · 2017-03-30T13:03:16Z

blocks/serializer.js

+		// We take all the block attributes and exclude the block attributes computed
+		// using the `attributes` from the Block Settings.
+		let contentAttributes = {};
+		if ( 'function' === typeof blockSettings.attributes ) {


Should we abstract this since it's now both here and in parser's getBlockAttributes ? (Or rather, should we just move getBlockAttributes somewhere more generic?).

We need to abstract getBlockContentAttributes and not the entire getBlockAttributes

aduth · 2017-03-30T13:05:32Z

blocks/serializer.js

+		} else if ( blockSettings.attributes ) {
+			contentAttributes = query.parse( rawContent, blockSettings.attributes );
+		}
+		const commentAttributes = Object.keys( block.attributes ).reduce( ( attrs, attribute ) => {


A Lodash _.difference on keys would be an easier way to find where there's differences between block state and its defined attributes 😉

aduth · 2017-03-30T13:06:39Z

blocks/serializer.js

+		}, [] );
+
+		// serialize the comment attributes
+		const serializedCommentAttributes = ! commentAttributes.length


Do you need this ternary at all? Seems like it would work just as well with:

const serializedCommentAttributes = commentAttributes.map( ( { key, value } ) => `${key}:${value}` ).join( ' ' ) + ' ';

I just wanted to avoid the extra space at the end when there's no comment attribute

I just wanted to avoid the extra space at the end when there's no comment attribute

There should be no extra space. The result of the map and join would still be an empty string if commentAttributes is empty.

I was talking about this one + ' '

Oh, I see now. I liked how this was handled transparently in the multi-instance prototype:

https://github.com/WordPress/gutenberg/blob/master/docs/tinymce-per-block/src/serializers/block/index.js

Here I suspect that would look like:

const serializedCommentAttributes = commentAttributes.map( ( { key, value } ) => `${ key }:${ value } ` ).join( '' );

Or:

const serializedCommentAttributes = commentAttributes.reduce( ( memo, { key, value } ) => memo + `${ key }:${ value } `, '' );

aduth · 2017-03-30T13:07:23Z

blocks/serializer.js

+ * @return {String}        The post content
+ */
+export default function serialize( blocks ) {
+	return blocks.reduce( ( memo, block ) => {


Do you think there's a few functions we could split out of this long block perhaps for easier testing?

As I noted above, it's hard to do without duplicating code (like computing rawContent and blockSettings several times) or passing those as parameters (which creates weird API)

youknowriad · 2017-03-30T13:08:14Z

@aduth I'm repeating and editing my answer here to ease discussion :)

I'm trying to be pragmatic here, we have several issues if we return an element (or a component) and I would like to move forward with this.

I think this resolves several issues:

Imagine using a component that requires lifecycle hooks, the rendering would be async.
~~How can we drop the data-react attributes easily~~ You resolved this remarkably above :P
We can not return multiple tags, A text block can have multiple paragraphs
Currently the content parsed by the text element returns all the markup test, if we add a container element, we would change the markup even if we do not perform any change test.
Why can't we re-use RandomImage to generate the same markup? Most of the time we won't be able to reuse any element.
What if category has a " in it? => you're right we'll have to escape it, but the opposite is true as well, we'll have to use dangerouslySetInnerHTML extensively with the element approach (The text block for example).

aduth · 2017-03-30T13:09:07Z

Rendering a string is synchronous, rendering an element could be async

In the context of server-rendering, of which this is comparable, there's no concept of asynchronous rendering and we never need to set the expectation that this would be possible to support.

aduth · 2017-03-30T13:09:29Z

We can not return multiple tags, A text block can have multiple paragraphs

React 16 will support array return values from functions. We should upgrade to the alpha.

aduth · 2017-03-30T13:11:44Z

What if category has a " in it? => you're right we'll have to escape it, but the opposite is true as well, we'll have to use dangerouslySetInnerHTML extensively with the element approach (The text block for example).

Then we should spend our effort trying to find ways to avoid dangerouslySetInnerHTML than giving developers different footguns to use.

aduth · 2017-03-30T13:15:24Z

Why can't we re-use RandomImage to generate the same markup?

Most of the time we won't be able to reuse any element.

I'd claim the opposite, that we should expect significant overlap between UI of the editor and what's saved.

youknowriad · 2017-03-30T13:15:39Z

I give up :) you have good points obviously (and I do too :P), but for the moment I can't be convinced. I feel like we're using components to generate markup and components are way more than markup for me.

aduth · 2017-03-30T13:18:56Z

I feel like we're using components to generate markup and components are way more than markup for me.

Yeah, I think my opinions differ on this point in my distaste for the Component class and lifecycle, and a preference to view React as a simple input/output mechanism, for which the original APIs work quite well.

aduth · 2017-03-30T15:50:44Z

blocks/serializer.js

+		}, [] );
+
+		// serialize the comment attributes
+		const serializedCommentAttributes = ! commentAttributes.length


Oh, I see now. I liked how this was handled transparently in the multi-instance prototype:

https://github.com/WordPress/gutenberg/blob/master/docs/tinymce-per-block/src/serializers/block/index.js

Here I suspect that would look like:

const serializedCommentAttributes = commentAttributes.map( ( { key, value } ) => `${ key }:${ value } ` ).join( '' );

Or:

const serializedCommentAttributes = commentAttributes.reduce( ( memo, { key, value } ) => memo + `${ key }:${ value } `, '' );

aduth · 2017-03-30T15:52:04Z

blocks/serializer.js

+		// serialize the comment attributes
+		const serializedCommentAttributes = ! commentAttributes.length
+			? ''
+			: commentAttributes.map( ( { key, value } ) => `${key}:${value}` ).join( ' ' ) + ' ';


I think we should enable: http://eslint.org/docs/rules/template-curly-spacing (always)

This seems consistent with our preferred spacing.

aduth · 2017-03-31T15:51:43Z

editor/editor/layout.js

@@ -17,8 +17,7 @@ class Layout extends wp.element.Component {
 	}

 	switchMode( newMode ) {
-		// TODO: we need a serializer from blocks here
-		const html = this.state.html;
+		const html = this.mode === 'text' ? this.state.html : wp.blocks.serialize( this.state.blocks );


Shouldn't this.mode on this and the following line be this.state.mode ?

Also, can we not store html and blocks in state and instead perform this logic in `render?

I'm performing this logic here for performance reasons. Only parse/serialize when switching.

And good catch for the this.mode error

The since-removed "props in getInitialState as anti-pattern" article spoke to this specific usage of state as a cache to often be a poor choice for the impact on maintainability caused by duplicating the source of truth:

Using props to generate state in getInitialState often leads to duplication of "source of truth", i.e. where the real data is. This is because getInitialState is only invoked when the component is first created.

Whenever possible, compute values on-the-fly to ensure that they don't get out of sync later on and cause maintenance trouble.

https://github.com/facebook/react/blob/f1c1544/docs/tips/10-props-in-getInitialState-as-anti-pattern.md

In our specific case, it also seems to assume that we don't have a good sense of when render will be called. As-is, we should expect it only to be called on initialization and on mode changes, which is where we're regenerating html and blocks state anyways.

Good point 👍

aduth · 2017-03-31T15:53:19Z

blocks/test/serializer.js

+ * Internal dependencies
+ */
+import { default as serialize } from '../serializer';
+import * as blocks from '../registration';


In #355 I tried to avoid treating registration as some canonical set of blocks properties (since wp.blocks encompasses more than just registration). I don't know how you feel about this. It's a small point.

It's better 👍

aduth · 2017-03-31T15:55:19Z

blocks/test/serializer.js

+
+describe( 'block serializer', () => {
+	// Reset block state before each test.
+	beforeEach( () => {


In #335 I changed cleanup to afterEach. I like beforeEach in the general case within a single isolated test suite, but our current setup is not very isolated with singleton blocks, to the effect that one test suite can potentially affect the other. Changing to afterEach was an attempt to ensure that each test will not leave any lingering effects to the next.

Authored changes

aduth · 2017-04-06T01:06:22Z

I've rebased and updated this pull request to account for state introduced in #368.

A major shift from #368 to better accommodate serialization is that HTML is no longer considered the source of truth, but rather the blocks themselves. In displaying the text editor, we assign the default value as the serialized form of the current blocks state. This simplifies state a fair bit.

I also added support for string return values from save as we talked about (8f684f821a18981180244005cfe00d955e166ddc). This helps avoid the need for a redundant div wrapper on the freeform save return value and is considered appropriate if the save is expected to return some raw markup string.

Textarea shown as serialization of current blocks state

Avoids parsing on every keystroke in text editor, rather only when existing field

youknowriad added the Framework Issues related to broader framework topics, especially as it relates to javascript label Mar 29, 2017

youknowriad self-assigned this Mar 29, 2017

youknowriad requested a review from aduth March 29, 2017 11:51

youknowriad force-pushed the add/serializer branch from 917f369 to 1da371f Compare March 29, 2017 11:53

mtias reviewed Mar 30, 2017

View reviewed changes

aduth reviewed Mar 30, 2017

View reviewed changes

aduth mentioned this pull request Mar 30, 2017

Find tests in nested directories #355

Merged

youknowriad force-pushed the add/serializer branch 2 times, most recently from cb90ea8 to 40de95e Compare March 30, 2017 14:31

aduth reviewed Mar 30, 2017

View reviewed changes

aduth previously requested changes Mar 31, 2017

View reviewed changes

youknowriad force-pushed the add/serializer branch from 9575f6f to 5502412 Compare March 31, 2017 16:03

aduth mentioned this pull request Mar 31, 2017

UI: Adding the selected and hover state for the visual editor #360

Merged

aduth force-pushed the add/serializer branch from ecc648b to c68e8e4 Compare April 6, 2017 01:02

aduth mentioned this pull request Apr 6, 2017

Two Types of Blocks for Better User and Developer Experience #371

Closed

youknowriad and others added 7 commits April 6, 2017 13:35

Serializer: Adding a block state serializer (blocks => postContent)

5ce0284

Flip source of truth to blocks first

e4d3086

Textarea shown as serialization of current blocks state

Defer change event to blur

31ccf60

Avoids parsing on every keystroke in text editor, rather only when existing field

Support string return values from save

3c84cc1

Reuse getBlockAttributes from parser

d1b75f7

Extract comment attribute serialization

0dfc163

Support save serialization by component type

20b7b94

aduth force-pushed the add/serializer branch from c68e8e4 to 20b7b94 Compare April 6, 2017 17:55

Document save return value as string

0db45f2

aduth merged commit f1b8d8d into master Apr 6, 2017

aduth deleted the add/serializer branch April 6, 2017 18:02

Serializer: Adding a block state serializer (blocks => postContent) #348

Serializer: Adding a block state serializer (blocks => postContent) #348

Conversation

youknowriad commented Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Mar 30, 2017

youknowriad commented Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Mar 30, 2017 • edited Loading

aduth commented Mar 30, 2017

aduth commented Mar 30, 2017

aduth commented Mar 30, 2017

aduth commented Mar 30, 2017

youknowriad commented Mar 30, 2017

aduth commented Mar 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth commented Apr 6, 2017

youknowriad commented Mar 29, 2017 •

edited

Loading

youknowriad commented Mar 30, 2017 •

edited

Loading

youknowriad commented Mar 30, 2017 •

edited

Loading