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

Try: Custom save serializer, Editable as array tree #4049

Closed
wants to merge 2 commits into from

Conversation

aduth
Copy link
Member

@aduth aduth commented Dec 15, 2017

Closes #3353
Closes #2750
Related: #3048, #2463, #3713

This pull request seeks to explore using a custom element serializer without markup escaping and with built-in support for the "vanilla" element tree described in #2463, enabling us to represent the value of Editable as a simple object type, not a React element tree (enabling better collaborative editing via serializable values, server-side block templating of editable attributes).

React doesn't expose hooks for its server serialization, so I instead resorted to a custom implementation, a modified version of a library which implemented a simplified server renderer.

@aduth aduth added Framework Issues related to broader framework topics, especially as it relates to javascript [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable labels Dec 15, 2017
@aduth aduth force-pushed the try/custom-serialize branch from e1fe602 to 20da270 Compare December 18, 2017 18:03
@ellatrix
Copy link
Member

Looks good to me at first sight, over #3048. Do you think this will be the way to go? I'll have a closer look in a bit.

@aduth
Copy link
Member Author

aduth commented Jan 2, 2018

Do you think this will be the way to go?

My only concerns currently are:

  • Ensuring that opting in to the dangerouslySetInnerHTML behavior by default is not... dangerous.
  • That there aren't any gotchas to the array structure. Specifically, one issue I encountered is that React's representation of children can look very similar to what we might consider as an element, e.g. [ "Hello", "World" ] is in fact a set of string children, but looks similar to how we might represent a node of tag name <Hello />. The isPropsLike function in serialize.js handles this, but in a very duck testing manner.
  • That we are creating our own separate node representation alongside React's. On its own I'm not terribly concerned since it solves many of the problems we've faced, but it introduces questions around (a) how we coerce between DOM, React, and our array node representation and (b) how far we take the array structure. For example, it's now technically possible to return an array from save instead of a createElement instance. Somewhat related to Try: Framework-agnostic block interoperability (Vanilla, Vue) #2463

@aduth
Copy link
Member Author

aduth commented Jan 4, 2018

Ensuring that opting in to the dangerouslySetInnerHTML behavior by default is not... dangerous.

We should also be conscious of the fact that, if implemented, this could be used outside the context of the editor alone, if we plan to use wp.element more broadly. At worst, I might imagine renderToString to accept an argument e.g. renderToString( escape = true )

A further concern is the fact that we are drifting more out of alignment with React, which as an abstraction layer is not a critical issues, so long as the changes are well documented.

@mcsf
Copy link
Contributor

mcsf commented Jan 4, 2018

I really like this experiment. My main concern right now can be illustrated by this screencast:

gutenberg-custom-serialize

A dissonance is introduced between what is seen in the editor and what is seen on the front-end. I think that's fine (and desirable) for entities like ampersands and quotes, but—as observed—things get picked up as HTML tags. The following example with a wrapping <code> tag is perhaps more telling:

screen shot 2018-01-04 at 15 29 43

@ellatrix
Copy link
Member

That there aren't any gotchas to the array structure. Specifically, one issue I encountered is that React's representation of children can look very similar to what we might consider as an element, e.g. [ "Hello", "World" ] is in fact a set of string children, but looks similar to how we might represent a node of tag name . The isPropsLike function in serialize.js handles this, but in a very duck testing manner.

If [ "Hello", "World" ] were an element, it would be [ "Hello", {}, "World" ]?

return null;
}

if ( node.nodeType === 3 ) {
Copy link
Member

Choose a reason for hiding this comment

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

We should use window.Node here.

{ value && value.map( ( paragraph, i ) =>
<p key={ i }>{ paragraph.children && paragraph.children.props.children }</p>
) }
{ value.map( ( paragraph ) => paragraph.children ) }
Copy link
Member

Choose a reason for hiding this comment

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

It's unfortunate that this still means that the block author needs to be aware of the editable value structure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, in this case this is the structure from the block attribute's query result, necessary because there's no wrapper to separate content from the citation that would allow us to use the children source directly.

@aduth
Copy link
Member Author

aduth commented Jan 11, 2018

If [ "Hello", "World" ] were an element, it would be [ "Hello", {}, "World" ]?

Yeah, this is more or less how the logic works to detect the array as an element vs. an array of elements / strings. In our structure, we'd try to enforce children as an array so more accurately it would be [ "Hello", {}, [ "World" ] ]

if ( Array.isArray( element ) ) {
if ( isPropsLike( element[ 1 ] ) ) {
const [ type, props, children ] = element;
return renderElement( { type, props: { ...props, children } } );
}

/**
* Returns true if the given argument can be inferred to be a props object.
*
* @param {*} object Object to test
* @return {Boolean} Whether object is props
*/
function isPropsLike( object ) {
return object && object.constructor === Object && ! ( '$$typeof' in object );
}

(I might want to look to see if React elements are plain objects, and if not, whether we can generalize this logic to testing as a plain object, e.g. _.isPlainObject)

@aduth
Copy link
Member Author

aduth commented Apr 6, 2018

Superseded by #5897

@aduth aduth closed this Apr 6, 2018
@aduth aduth deleted the try/custom-serialize branch January 25, 2019 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants