Text Editor: Fix inserting blocks

[youknowriad]

closes #1072

This bug is way harder to solve than it seems because for performance reasons, we want to avoid calling parse/serialize as much as we can.

The solution here involves:

• Making the text editor controlled
• Tracking blocks prop updates and serialize the new content each time the prop change
• Make sure the getBlocks selector returns the same array instance if the state didn't change
• Trigger a "MARK_DIRTY" action each time we change the text editor value (to enable the publish button) even if the blocks value is not dispatched to the store.

[aduth]

I recently published a Calypso-like memoization library: https://github.com/aduth/rememo

Curious if you have thoughts on whether reselect is a better approach or not.

[youknowriad]

To be honest, I don't have any preference. The advantage of reselect is that it's kind of the default with Redux, more developers already know how to use it. But at the same time, rememo is probably easier to learn.

[aduth]

Fair point. I've always found the examples they chose in their documentation to be overly daunting for the sake of showcasing a complex example, but what you've shown here is much simpler.

Another worry is that they don't really prescribe easy patterns for accepting arguments to a selector:

https://github.com/reactjs/reselect#q-how-do-i-create-a-selector-that-takes-an-argument

Here worked out easy enough because getBlocks doesn't accept any anyways.

[youknowriad]

mmm! Good point about the selectors with arguments, I think we'll need this quickly. Though according to their docs, we could write this

const createSelectorWithArguments = ( extractors, resolver ) => {
	const memoizedResolver = memoize( resolver );
	const memoizedSelector = createSelector( extractors, memoizedResolver );
	return ( state, ...args ) => {
		const selector = memoizedSelector( state );
		return selector( ...args );
	};
};

[youknowriad]

mmm looks like https://github.com/toomuchdesign/re-reselect do exactly what I've proposed.

But I think I'll just switch to rememo

[youknowriad]

Actually! forget my implementation it's completely broken I think :)

[youknowriad]

@aduth I think there's a problem somewhere in rememo. I've replaced the selector by this:

export const getBlocks = createSelector(
	( state ) => {
		return state.editor.blockOrder.map( ( uid ) => (
			state.editor.blocksByUid[ uid ]
		) );
	},
	state => state.editor.blockOrder,
	state => state.editor.blocksByUid,
);

and I'm getting a new Instance even if the state didn't change.
I'll probably keep reselect for now, and feel free to update once fixed.

[aduth]

@youknowriad That's incorrect usage. There should be two arguments at most, so the second should be changed to:

( state ) => [
	state.editor.blockOrder,
	state.editor.blocksByUid,
]

[youknowriad]

Still not working 🤔

[youknowriad]

updated with the last version of rememo it works great.

[jasmussen]

Unrelated to the conversation above, this seems to be working well! Thanks for tackling this so fast.

[aduth]

As noted in chat, can we test here whether the value has changed from what we know to have been the previous value before parsing and assigning new blocks?

[aduth]

Total aside but on the note of memoized selectors: I've been quite intrigued by Vue's approach for change tracking using Object.defineProperty, and I feel like there could be an easy pattern for memoization in Redux applying the same technique; i.e. analyzing the structure of state, replacing each key (and subkey) with a getter, and detecting calls to these properties within the selector to determine when the cache needs to be busted (effectively replacing the need for the second argument to createSelector). I might do some explorations on this separately.

[aduth]

Steps to reproduce:

1. Navigate to Demo
2. Switch to Text
3. Insert block

Expected: Serialized block content appended to the end of content
Actual: Serialized block content inserted between first and second blocks, regardless of where focus changes within text

Guessing our visual editor mode insertion point is interfering.

[youknowriad]

Good catch @aduth. I've addressed the issue by simplifying the insertion point behaviour and moving the logic to a testable selector in 44f6fe1

[aduth]

I like the refactor. This works well now 👍