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

Experimental: Remove children matcher in favor of HTML #1907

Closed
wants to merge 1 commit into from

Conversation

aduth
Copy link
Member

@aduth aduth commented Jul 14, 2017

Related: #421, #466

The children matcher was introduced in #466 as a solution to the issue where serializing content as a React element in the save implementation would require the block implementer to use dangerouslySetInnerHTML, which was unnatural (and scare-inducing) compared to typical React children rendering. The result of save is only ever used to generate the string content to be saved in post content, so escaping the rich HTML of an Editable field value is undesirable.

This pull request seeks to explore an alternative solution, preferring to manage the Editable (TinyMCE) value as a string, and overcoming the issue of dangerouslySetInnerHTML by intercepting and traversing the return value of save to replace a string children prop with the equivalent dangerouslySetInnerHTML. The magic occurs in serializer.js:

content = traverse( content, {
	DOMElement( { node, traverseChildren } ) {
		if ( 'string' === typeof node.props.children ) {
			return cloneElement( node, {
				...node.props,
				dangerouslySetInnerHTML: {
					__html: node.props.children,
				},
				children: undefined,
			} );
		}

		return cloneElement( node, node.props, ...traverseChildren() );
	},
} );

See also: https://github.com/elierotenberg/react-traverse

This has a few benefits:

  • Fewer matchers to have knowledge of
  • Avoids nodeListToReact / renderToString dance in Editable (more performant)
  • Editable change detection is a simple reference comparison (more performant and accurate than deep isEqual on element trees)
  • Eliminates concatChildren because concatenating values is a simple addition operator with strings

This is a work-in-progress, notably:

  • Transforms need to be updated to treat children content as string
  • Security audit; I'd like to see if we could at least avoid the dangerouslySetInnerHTML in the TinyMCE component and instead set content via traditional TinyMCE setContent (with sanitization)

@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 [Component] TinyMCE labels Jul 14, 2017
@aduth aduth force-pushed the remove/children-matcher branch from 97e9ef1 to 253fef8 Compare July 26, 2017 12:47
@aduth
Copy link
Member Author

aduth commented Aug 8, 2017

I'm going to close this as not worth pursuing. Managing content as strings solves some problems, particularly around storing elements in state and serializing them (e.g. websockets), but at the cost of flexibility of understanding, slicing, mapping the true shape of the content.

@aduth aduth closed this Aug 8, 2017
@aduth aduth deleted the remove/children-matcher branch August 8, 2017 15:28
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.

1 participant