Manage Editable value as React element

[aduth]

Closes #421

This pull request seeks to address issues with treating the Editable value as a string. Specifically, the need to manage a string value counteracts the promise of avoiding manual manipulation of HTML and adds complexity to serialization.

Related discussion at #413 (comment)

Implementation details:

• Introduces a new children hpq matcher to extract content as a React element, using html-to-react
• Enhances renderToString to handle more incoming value types (falsey, string, array)

It's admittedly concerning to apply even more parsing in the form of HTML → React transformation, especially in juggling the incoming and outgoing value on the Editable (TinyMCE) component. I think we'll need to address this sooner than later, but the changes introduced here are aimed at shaping expectations of managing attributes of a block which contain elements.

Testing instructions:

Verify that there are no regressions of existing behavior:

• Confirming serialization of changes are respected (switching to HTML mode)
• Text blocks can contain multiple paragraphs
• Text block splitting

Ensure included tests pass:

npm test

[youknowriad]

Should we setContent( '' ) instead? What if it's an update?

[aduth]

Should we setContent( '' ) instead?

Oh, yes, I think I meant for this line to be content = '';

[aduth]

Fixed in rebased d4f5a57c4854c11d405691b37824e63cefb3eab1.

[youknowriad]

Could this be something different than an array of undefined ?

[aduth]

Could this be something different than an array of undefined ?

Yes, I've added some clarifying comments and slightly adjusted the behavior to return early on an undefined content in the rebased d4f5a57c4854c11d405691b37824e63cefb3eab1.

Specifically:

// Content can be in the following shapes, so we normalize to an array:
// - Single paragraph: Object
// - Multiple paragraph: Array of objects

[youknowriad]

When do we have a string here?

[aduth]

When do we have a string here?

When splitting a paragraph into two, the default TinyMCE getContent was including a newline between the paragraphs which when parsed by html-to-react resulted in an [ Element, '\n', Element ]. In the rebased 075237ef5d3947a9f784c27580bf6ba9a9193a4e, I changed this to get content as raw formatted with the assumption that clean-up will occur as a result of html-to-react and renderToString (see also extended commit description).

[youknowriad]

Should we be concerned about performance here? renderToString on each render and parse on each onChange ?

[aduth]

Should we be concerned about performance here? renderToString on each render and parse on each onChange ?

Yes 😄

[aduth]

Not the intent of the change, but a trade-off that could help performance slightly is change to retrieve content from editor instance as raw formatted in 075237ef5d3947a9f784c27580bf6ba9a9193a4e.

[youknowriad]

could we use castArray

[youknowriad]

The thing that bothers me with this, is that Plugin authors would need to understand well React Elements.

The fact that it could be sometimes a single element and sometimes an array is a bit confusing I think. Shouldn't we always castArray in the parser and in the Editable?

Anyway, this solves the dangerouslySetInnerHTML issue and is worth a try

[youknowriad]

Honestly, I don't see much value in abstracting "React"

[aduth]

Honestly, I don't see much value in abstracting "React"

What are you suggesting here?

[youknowriad]

we just drop wp.element :)

[aduth]

I could probably be convinced. Needs to be a decision made with future compatibility in mind, related to some of the "reason to be" outlined in the docs:

https://github.com/WordPress/gutenberg/tree/master/element#element

[youknowriad]

To clarify my point, I don't see much value in abstracting React because we're using a lot of React features: Element, Component (refs, state, lifecycle), cloneElement, renderToString, render. Which is close to be the whole React.

Also, and this is probably the main reason for me. If we want to use a library like react-textarea-autosize or react-tooltip or any library dependent on React, we can do so (we're already doing it) technically and it'll work because we're using React but what if we switch our wp.element implementation to a custom React alternative, nothing guarantees that the libraries we're using are only using the React features we whitelist on our implementation. And this IMO should force us to avoid using any React third-party library because "logically" they're not dependent on our abstraction but they're dependent on React (which is a larger library).

[aduth]

The thing that bothers me with this, is that Plugin authors would need to understand well React Elements.

I don't anticipate that they'd need to do any manipulation of the object, and I'm fine with it being slightly opaque in that regard. You're correct there would need to be some awareness of what the value is generally.

The goal here is to eliminate plugin author consideration of markup strings. We could simply pass the rawContent of a parsed block to the implementation and call it a day, but we've decided there's some value in forcing implementers to think of their block in terms of its data shape. That we still maintain HTML strings regresses away from this ideal (and almost encourages doing so).

So the question becomes: if we want to represent the caption, citation, or paragraphs nested elements as data, what shape does it take? I think there are some less complex options if we optimize for the value's shape to be inspected, but I don't think we care. A React tree seemed a natural fit into what we're already building.

Aside: I'm not claiming you disagree with me on any of these points. Just trying to clarify (for myself and others) the rationale for this change.

[aduth]

The fact that it could be sometimes a single element and sometimes an array is a bit confusing I think. Shouldn't we always castArray in the parser and in the Editable?

I'm not totally clear on the ramifications of this change, but I think it could make sense, sure.

[aduth]

Rebased again to resolve conflicts, and account for newly added Embed block.

@youknowriad 0924fe6 is an improved approach to handling differing shapes of the parsed content, leveraging the fact that React.Children exists for this very purpose:

utilities for dealing with the this.props.children opaque data structure

https://facebook.github.io/react/docs/react-api.html#react.children

And yes, I can totally appreciate that including this adds fuel to your argument that we should drop the abstraction 😆 Let's do that... but not here.