Editor: Sync blocks state to edited post content

[aduth]

Blocked by: #9287 (merged)
Fixes regression introduced in #9288 (#9288 (comment)) (Fixed separately by #9448)
Related: #7970

This pull request seeks to refactor how blocks state is kept in sync with the edited post content, in an effort to simplify / improve consistency of behaviors around editor content initialization.

Status: This pull request is rather large and includes a few changes that could potentially be forked out into their own, blocking pull requests (see commits as standalone indicators). It is otherwise expected to be fully functional and passing all tests. New end-to-end tests are planned to verify associated code editor impact, but there are presently issues with the local Docker installation, making end-to-end test authoring impossible for the moment.

Goal: The editor module needn't be so conscious of distinct start and end points. It should render itself as derived by its state at any given moment in time. This should help enable efforts like that of #7970, where the editor may need to present itself without relying on a synchronous initialization setup. It is made challenging by various factors such as: enacting a template, assigning initial edits, alerting the user to the presence of an existing autosave which can be restored. To me, these are considerations which should be moved into the scope of edit-post. This has its own set of challenges, and is proposed to be addressed separately to the work here.

Changes:

• Deprecating setupEditor, setupEditorState, and checkTemplateValidity actions from the editor module
  • Template validation occurs automatically as an effect of RESET_BLOCKS
• Refactoring overridePost as representing an initial set of edits (for demo content, auto-draft edits)
  • This also avoids needing to account for the auto-draft initial title edit in the editor effects
• Avoid initial history for ignored types in withHistory higher-order reducer
  • cc @iseulde , I cannot recall the original reason for this behavior, and while it's sensible to have the initial value to revert back upon as the "starting" history, it doesn't follow that ignored types option would be disregarded to assign this initial value. As it impacts the changes here, without the change it was causing an initial Undo option to be shown when loading the editor.
• Fix bug with how we consider prop and state relationship of edited content value in PostTextEditor

Implementation notes:

• This changes how we consider the source of truth for post content.
  • Previously there was an awkward juggling of edits, where if content was assigned as edits (as in by the Code Editor textarea), it would be preferred. This was to accommodate the fact that parsing content is a non-performant task to perform on each key press, thus the parse was deferred. However, this introduced a dual source of truth for content (sometimes as a serialization of blocks state, sometimes as the edits.content property).
  • With these changes, blocks are always considered as the source of truth for content, and the state value is synced automatically to any changes which occur to the post's edited or persisted content string (via the newly-introduced middleware).
  • To maintain existing Code Editor behavior, an optional flag is supported in the sync middleware to skip the parse. It's assumed this would be followed by a later deferred parse; in the code editor, on the blur of the text field.
    • However, this has a few consequences on how the Code Editor behaves:
      • If the post was not saveable because it had no title, content, or excerpt, and I add content in the Code Editor, it will not be reflected in the UI (by presence of the save button) until I blur the field.
      • If autosave occurs while I am within Code Editor, the saved payload will not include the in-progress edits, but rather the value of content before I began editing. If I then blur the field, the editor will correctly show as having changes needing to be saved (the deferred parse). One exception is if I press Cmd+R (reload) while within the code editor, I may not be prompted about unsaved changes because the deferred parse has not yet occurred by that time. This could probably be remedied by enhancements proposed by State: Refactor isEditedPostDirty to consider presence of edited properties #7409.
    • Considering the goal in consolidating how we consider post content, these changes aren't technically unexpected, but have the potential for a negative impact on user experience.
• This introduces an affordance of "quiet" state changes. We already have a number of state mechanisms and initialization steps which cause state values to change, but for which we don't want to prompt the user about unsaved changes / add history levels. This can be seen with demo content and the auto-draft initial edits, where the initial edits should not be considered as "unsaved", nor should they be undo-able.

Future tasks:

• Consider moving more behaviors which continue to live in EditorProvider into the edit-post module, namely template synchronization and the autosave warning.
  • It's unclear whether "template" should be considered a setting of the editor, as there is mixed expectations on when it is synchronized. Is it just a starting point for a new post? Do we apply the template when editing an existing post which does not adhere to the template? This has the potential for being destructive, and if we were to consider changing it to a user behavior, it would seem to follow that the editor doesn't need to be aware of some ongoing template, since the enacting of the template synchronization would be an explicit action.

Testing instructions:

Verify there are no regressions in the initialization of the editor, notably:

• New post without template
  • Should not show "Auto Draft" title
  • Should not save an "Auto Draft" title, even if title is left empty
• New post with template
• Editing existing post with content
• Editing existing post where template would exist, but content differs from template (see also testing instructions from Templates: Apply template for new post only #9288 )
• Demo post

Verify that HTML edits made in the Code Editor are respected:

• Post should be marked as dirty after focus leaves the textarea field
• Switching back to Visual Editor should display blocks as edited in Code Editor

[youknowriad]

Why two options to ignore actions?

[aduth]

This is definitely "sugar". In fact, in retrospect (or an earlier alternate branch) I'd even expressed isIgnored as a composition of ignoreTypes using _.overSome.

[aduth]

Neglected to finish my thought: I've personally found we are heavy-handed in using ignoreTypes for things which don't obviously seem warranting ignore. This also serves the purpose of allowing an action to be extended with additional metadata to trigger the ignore (which is what we're doing here with considering "programmatic changes" as something to ignore).

[youknowriad]

Won't this better achieved with getDerivedStateFromProps ?

[aduth]

Hm, I guess that would be a matter of returning value as state using props value when... the state is set to null ? I can give it a try.

[youknowriad]

Should we move this to the Editor component's did mount instead to avoid relying on dispatch as a global?

---

My thinking also is that ultimately we should get rid of initializeEditor function entirely, It should be just an Editor component exposed in edit-post

[aduth]

My thinking also is that ultimately we should get rid of initializeEditor function entirely, It should be just an Editor component exposed in edit-post

That's an interesting thought! It also solves another issue where it's hard to extract things like template synchronization out of EditorProvider because it relies on the post entity having been resolved; something we can't do from initializeEditor, but we could do from the (edit-post) Editor component.

[youknowriad]

This makes me think, this should be a local state and only called when onPersist is called.

[aduth]

Perhaps.

Good: It might help us avoid the need for this skipContentParse flag altogether.
Bad: We rely on editPost to trigger the change detection which would alert the user to their changes being unsaved. As noted in the original comment, this isn't entirely perfect as implemented anyways.

[youknowriad]

I'm not certain I follow why we need this?
Is it an optimistic update, if it's the case should we add the optimist: { id: POST_UPDATE_TRANSACTION_ID } ?

[aduth]

It's probably not a good sign that my initial thought is I didn't quite remember why I added it. I'll see if I can improve the comment.

I think the issue was that without it, the content was being considered as an unsaved edit because we compare what we receive via updatePost against edits, keeping those which differ.

[youknowriad]

Why do we need this to be a middleware? You probably guessed that I don't like middlewares too much :P because I think they obfuscate code even more than effects :). Also, it's one other pattern that may be replaced by something else.

If I understand properly it's a middleware for two reasons (correct me if I'm wrong):

• Because it's not really triggered by a single action but it's more triggered by a change in the state
• Because it needs to perform side effects (async stuff)

Did you consider:

• Making it a higher-order reducer. (I guess the argument against this is the upcoming async parser, instead of a required sync parser in reducers)
• Making it an effect? How many actions need to trigger this?

[youknowriad]

Can't this be a combination of an effect (adding the parsed content to the required actions) and a higher-order reducer using the parsed content?

[aduth]

Yes, my original intention was that this didn't fall neatly into our other patterns (effects, generators) because it doesn't care which action triggered the change, it only cares about the change in the content state values. In retrospect, I'd probably agree that this is the exact sort of unpredictable behavior we might want to discourage.

An original thought I'd had was to support some kind of action creator to set content using our new routines generators (enabling both our current sync parse and a future async parse). Roughly:

function* setContent( content ) {
	const blocks = yield { type: 'PARSE', content };
	yield resetBlocks( blocks );
}

In thinking more on it, it seemed a bit redundant considering we have other actions which operate on any edit to a post property; having a separate one for "content" seemed out of place.

Should those action creators do the parse when the payload includes content ? What are those action creators? editPost, updatePost, resetPost ... ?

Worth thinking on more...

[aduth]

Thanks for the review @youknowriad . I'm still liking the overarching goal here, but finding some of the compromises to have introduced new confusions / shortcomings. I want to spend more time thinking on how best to move forward here (your input very welcome). In the meantime, we'll need a more immediate solution for #9433. I'll see if there's an option immediately available to us, otherwise we should fall back to a simple revert of #9288 .

[aduth]

3.9 🤦‍♂️

[youknowriad]

Granted I don't like the middleware approach, but I think personally, I like the direction this PR is going.
Even if we decide to revert for now, I don't mind following this approach for the future releases.

[aduth]

Rebased to resolve conflicts.

Couple updates:

• Initial edits applied in (edit-post) Editor component
• Use corrected version for deprecation (4.0)

Current plan is to fork out a couple separate pull requests:

• Deprecate checkTemplateValidity and perform validation on RESET_BLOCKS
• Bug fix to PostTextEditor's treatment of state / props value

In parallel, I'll continue to think on when and how the content -> blocks sync should occur (substitute for current proposed middleware).

[aduth]

In considering how this enables #7970, I think it helps to consider how an asynchronous parse highlights the need for intentionality around parse times, particularly in disruptiveness to user flow. If we imagine that the editor enters into a read-only state while undergoing the parse (so as to avoid destructiveness / syncing concerns relating to changes made during the parse), it becomes evident that there are only few occasions under which we expect a parse to occur; namely, when we change entirely the context of the post being edited. In terms of actions, this would be the resetPost action. In practical terms, this is the start of the editor session, though it's important to note this is more incidental so as not to bias the most correct implementation.

With that in mind, it becomes clearer that the parse doesn't need to act as a middleware responding to any change in content, but rather as part of the routine of resetting the post. My recommendation here therefore would be to call parse as part of the resetPost action (ideally as a yielded generator action, so support the future asynchronous use-case).

One issue we'll encounter here is that currently, we call to resetPost when a save completes. This doesn't align well with our expectations that blocks be parsed; we'd neither want a read-only state, nor would we want the blocks to suddenly become replaced with another set per the reset post. If I were to try to recall why we chose resetPost here, I expect it's because we receive the entire post entity, and it appeared a good conceptually fit that we "shove" the entire post into state, rather than call updatePost which is otherwise used only for subsets of properties. My recommendation here therefore would be to change this usage to updatePost.

[aduth]

Current plan is to fork out a couple separate pull requests:

• Deprecate checkTemplateValidity and perform validation on RESET_BLOCKS
• Bug fix to PostTextEditor's treatment of state / props value

The second was addressed and merged in #9513. The latest rebase here reflects this.

I am working on the first as I speak, and will follow-up with a pull request reference.

[aduth]

Template validation refactor in #9916

[aduth]

Extracted client-assets.php refactoring / improvements to #9921

[aduth]

With #9916 and #9921 having been merged, I've rebased here to resolve conflicts.

To be clear, this is not quite ready yet, as I need to implement the revised approach as described in #9403 (comment) .

[aduth]

One of the original objectives here was in trying to eliminate a setup sequence for the editor. As I've since discovered in #11267, this may in-fact be inevitable in order to support both post canonical date and initial edits, while also ensuring that at most a single parse of content occurs when content "changes".

#11267 includes a number of the revisions which had been proposed here, including an explicit treatment of initialEdits as part of the public interface of edit-post, and the dispatching of notice actions within EditorProvider (out of the effect handler for setup).

I would have liked to be able to at least consolidate SETUP_EDITOR and SETUP_EDITOR_STATE, but given the machinery around history and change detection resets, I'm not sure this is achievable in the near future.

We may, however, be able to consolidate some of the redundant handling between SETUP_EDITOR_STATE and RESET_BLOCKS, a task described in more detail at the issue #11641 I've just now created.

With all this in mind, I think this pull request no longer serves much value in its current form.