Text Block: Fix updating the content of an empty text block

[youknowriad]

Treating Editable's value as a React Element introduced a regression when updating the content of an empty text block. The content of the text block was constantly being reset to "empty" because the updateContent was called too many times.

This fix may cause a performance loss (not noticeable right now for me).

[aduth]

Yeah, twice renderToString on update is a little concerning 😬

Part of the problem might be that we're creating a new paragraph element each time edit is called if the content attribute is empty, which is why value is not strictly equal in the first componentDidUpdate check:

https://github.com/WordPress/gutenberg/blob/f58fea4/blocks/library/text/index.js#L48

There might be a way to move this out of the edit function, but seems like an easy trap for a developer to fall in.

Since I'm seeing edit being called because of the global state's change in focus, maybe we should move this up the chain a bit to prevent it being called unnecessarily. Or maybe there's another way to compare the element trees of the value (a deep isEqual maybe)?

Edit: Ah, I guess it's not a totally unnecessary render, because we're passing the focus prop along to the Editable.

[aduth]

A few more issues that are potentially related:

• In Editable's onNodeChange, we're always calling onFormatChange even if the internal formats object has not changed (leading to an infinite loop as edit -> updateContent -> onNodeChange -> onFormatChange -> edit)
• Not confirmed yet, but I'm wondering if updateContent is also triggering a focus change, leading to another infinite loop.

[aduth]

#507 seems to resolve the infinite loop I'm seeing when first adding the new text block.

[youknowriad]

Should we merge this as is (it fixes a bug avoiding the usage of empty text blocks) and address performance issues later. Maybe related to #523

[aduth]

The main problem is simply that we're creating a different reference for content every time edit is called. I think we could avoid this by either moving the default to the parse step or referencing a constant. I'd wanted to avoid any explicit "defaulting" APIs since destructuring is a nice convention to avoid it, but it's also wasteful when we know the default will be the same every time.

I toyed with this in a separate branch with acbcc90 and aca7a04, adding a defaultAttributes property to the block settings API. This solves the issue the same as the approach here does.

Alternatively, I'm starting to question whether we need componentDidUpdate to check for and update the content. What's the use case? Ideally the flow should be that the block keeps its own state in sync using Editable's onChange callback. I guess from a purely "controlled component" perspective, it's nice that the two are guaranteed to be kept in sync, but detecting changes on element trees is problematic.

Aside: I rebased and force pushed the branch to account for changes merged in #507.

[youknowriad]

Alternatively, I'm starting to question whether we need componentDidUpdate

This is needed when splitting/merging blocks because the update won't come from the block itself. It's also needed in undo/redo.

---

I'm Ok with the defaultAttributes solution but It feels less safe for me, we do not control the external blocks.

[aduth]

We could do a combination of the two where we prefer to do a strict equality test, and only in the case that they differ, do a deeper check to verify that the content has indeed changed, either with renderToString or another means of comparing the trees (would Lodash's deep isEqual work?).

So condition becomes:

value !== prevProps.value &&
! deepEquals( value, prevProps.value )

[youknowriad]

@aduth Tried and seems to work properly

[aduth]

Just worried about some of the internal properties that React assigns, like key, where content could still be effectively the same but isEqual won't respect it. Probably fine since it's just capturing more than it really needs to, but maybe at that point isEqual isn't doing anything at all.

Do you still think there's value in the changes explored in my fix/empty-text-block-alt branch?

[youknowriad]

@aduth I think the defaultAttributes is a good feature regardless of the fix here. We need something like this to populate attributes of newly inserted blocks.

[aduth]

Brain-dump of my considerations. defaultAttributes seemed most obvious to expose block implementers to the idea of default values, but the same could have been achieved with:

1. attributes function:

attributes: ( rawContent ) => {
	return {
		content: parse( rawContent, children() ) || <p />
	};
}

2. Constant

const DEFAULT_CONTENT = <p />;

// ...

edit( { attributes } ) {
	const { content = DEFAULT_CONTENT } = attributes;

	// ...
}

Not sure if either of these sound more appealing to you.

[aduth]

Also, there's alignment with React's defaultProps, though... that's another feature whose future I am not so certain.

[youknowriad]

I have a preference for defaultAttributes because it seems "obvious".

I like the flexibility of the attributes as a function but it seems that for the different parts of the block API, we prefer guiding the block author as much as we can (better for novice developers, maybe less for experienced one).

[aduth]

Pushed default attributes behavior. This LGTM now if you agree to these changes 👍