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

Block Editor: Consider received blocks state change as ignored #14916

Merged
merged 4 commits into from Apr 11, 2019

Conversation

@aduth
Copy link
Member

commented Apr 10, 2019

Fixes #14910
Fixes #14766

This pull request seeks to resolve an issue where any fetch of reusable blocks (selecting a paragraph, using inserter, editing a post containing reusable block) would cause the post to immediately become "dirty" (prompt the user about unsaved changes when navigating away) and would immediately add an undo history level.

The changes here effectively better align to behavior of RECEIVE_BLOCKS prior to #13088 . As of #13088, we considered RECEIVE_BLOCKS as "non-persistent" which helped for avoiding to create undo levels (except when it's the first action of a session), but did not consider that received blocks should be entirely ignored in consideration of change detection (as it was implemented before #13088).

Testing Instructions:

Repeat Steps to Reproduce from #14910 and #14766, verifying expected results.

Ensure unit tests pass:

npm run test-unit packages/e2e-tests/specs/change-detection.test.js

Ensure end-to-end tests pass:

npm run build && npm run test-e2e packages/e2e-tests/specs/change-detection.test.js
@@ -775,6 +775,20 @@ via its `onChange` callback, in addition to `onInput`.

Whether the most recent block change was persistent.

### __unstableIsLastBlockChangeIgnored

This comment has been minimized.

Copy link
@nerrad

nerrad Apr 11, 2019

Contributor

hmm... I didn't think __unstable or __experimental was supposed to show up in docs?

This comment has been minimized.

Copy link
@aduth

aduth Apr 11, 2019

Author Member

hmm... I didn't think __unstable or __experimental was supposed to show up in docs?

It caught me off guard as well. I think they may only be omitted in documentation generated by the docgen tool. The data modules documentation is a separate process. It should be improved (I'll create a follow-up task).

*/
const IGNORED_ACTION_TYPES = new Set( [
'RECEIVE_BLOCKS',
] );

This comment has been minimized.

Copy link
@nerrad

nerrad Apr 11, 2019

Contributor

so for now is this just going to be a hardcoded list? Would there be value in being able to customize what action types are ignored? Eg. being able to call it like this:

withIgnoredBlockChange( [ 'RECEIVE_BLOCKS' ] )

If so, and that's useful, then the name of the HOC could maybe be changed to withIgnoredActionChange?

This comment has been minimized.

Copy link
@nerrad

nerrad Apr 11, 2019

Contributor

Re 'RECEIVE_BLOCKS being ignored, does that mean we expect change to be ignored for any block addition to the editor (and I'm assuming changes would get picked up when there is content added to the block?)

This comment has been minimized.

Copy link
@aduth

aduth Apr 11, 2019

Author Member

Re 'RECEIVE_BLOCKS being ignored, does that mean we expect change to be ignored for any block addition to the editor (and I'm assuming changes would get picked up when there is content added to the block?)

So, considering all of the above: Our use of RECEIVE_BLOCKS is and always has been exclusively a way to get blocks data for reusable blocks into state so they can be treated the same as any other content block by selectors such as getBlock, etc. But they're not part of the actual content. Regular user workflows to add blocks will go through INSERT_BLOCKS and friends.

Long-term then: RECEIVE_BLOCKS shouldn't be ignored, this list won't be necessary. We probably don't need to optimize too much for it now. In fact, RECEIVE_BLOCKS could probably be removed altogether as part of #7119, if not for compatibility's sake. Once reusable blocks are refactored as an embedded editor, we'd probably remove all of this ignoring behavior, and treat it as a real change that should trigger change detection.

In the meantime though, this basically restores the behavior we had prior to #13088 rather faithfully (including all imperfections therein).

// TODO: This should be considered a temporary test, existing only so
// long as the experimental reusable blocks fetching data flow exists.
//
// See: https://github.com/WordPress/gutenberg/issues/14766

This comment has been minimized.

Copy link
@nerrad

nerrad Apr 11, 2019

Contributor

Very good comment block explaining things 👍

const nextState = reducer( state, action );

if ( nextState !== state ) {
nextState.isIgnoredChange = IGNORED_ACTION_TYPES.has( action.type );

This comment has been minimized.

Copy link
@youknowriad

youknowriad Apr 11, 2019

Contributor

so what is exactly the difference between isIgnoredChange and ! isPersistentChange?

This comment has been minimized.

Copy link
@aduth

aduth Apr 11, 2019

Author Member

From __unstableIsLastBlockChangeIgnored docs:

An ignored change is one not to be committed by BlockEditorProvider, neither via onChange nor onInput.

From isLastBlockChangePersistent docs:

A persistent change is one committed by BlockEditorProvider via its onChange callback, in addition to onInput.

Effectively: Non-persistent changes are still surfaced up to the rendering Editor, but treated as omitted from history. Ignored changes are not surfaced to the rendering Editor at all.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Apr 11, 2019

Contributor

Oh I think I got it, while one is about the onChange vs onInput the other is about not calling them entirely.
It does seems like they have the same roles though. Avoid undo levels, can't we unify and just use one?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Apr 11, 2019

Contributor

(I replied to myself before I see your comment)

This comment has been minimized.

Copy link
@aduth

aduth Apr 11, 2019

Author Member

It does seems like they have the same roles though. Avoid undo levels, can't we unify and just use one?

This is a good summary of the problem. They're not the same. One is considered by withHistory, the other by withChangeDetection. The bug exists because we only accounted for the first, not the second.

If the question is whether "changes" should be a reflection of the presence of undo levels: I think long ago we had it this way, and it changed (presumably necessarily). Possibly as a result of some of the work to merge undo levels while still needing to consider them as changed.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Apr 11, 2019

Contributor

👍 Thanks for the clarification, it makes sense.

This comment has been minimized.

Copy link
@aduth

aduth Apr 11, 2019

Author Member

If the question is whether "changes" should be a reflection of the presence of undo levels: I think long ago we had it this way, and it changed (presumably necessarily). Possibly as a result of some of the work to merge undo levels while still needing to consider them as changed.

I think the reason we don't consider Undo levels as part of post dirtiness is because Undo history doesn't reset when a Save occurs, but the post is "clean".

@youknowriad
Copy link
Contributor

left a comment

It fixes the issue in my testing. Hopefully we could find a solution for the embeded reusable blocks editor at some point and remove these hacks.

@nerrad

nerrad approved these changes Apr 11, 2019

@aduth

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

Thanks @youknowriad, @nerrad . Either Travis is being particularly stubborn today, or there may be a legitimate issue. I'll give it another reset.

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

This particular e2e test (deletion I think) with plugins enabled is unstable. Without plugins it's pretty stable though, so might be something to do with the plugins we're installing.

@aduth

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

This particular e2e test (deletion I think) with plugins enabled is unstable. Without plugins it's pretty stable though, so might be something to do with the plugins we're installing.

Yeah, that deletion failure is quite nonsensical. The failure is that the content contains a reusable block, but the test itself is pretty straight-forward and involves no reusable blocks.

The other test with Undo, I could see some potential for race conditions prior to these changes, since the undo level could fluctuate depending on whether or not the reusable blocks fetch had time to complete. I would expect that should be improved here (fetch won't ever add the undo level).

@aduth aduth merged commit 8722f49 into master Apr 11, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@aduth aduth deleted the fix/14910-block-editor-receive-blocks-change branch Apr 11, 2019

mchowning added a commit to mchowning/gutenberg that referenced this pull request Apr 15, 2019

Block Editor: Consider received blocks state change as ignored (WordP…
…ress#14916)

* Block Editor: Avoid creating new state reference on unchanging isPersistentChange

* Block Editor: Consider received blocks state change as ignored

* fixup a35fa82

* Block Editor: Regenerate documentation

aduth added a commit that referenced this pull request Apr 15, 2019

Block Editor: Consider received blocks state change as ignored (#14916)
* Block Editor: Avoid creating new state reference on unchanging isPersistentChange

* Block Editor: Consider received blocks state change as ignored

* fixup a35fa82

* Block Editor: Regenerate documentation

aduth added a commit that referenced this pull request Apr 16, 2019

Block Editor: Consider received blocks state change as ignored (#14916)
* Block Editor: Avoid creating new state reference on unchanging isPersistentChange

* Block Editor: Consider received blocks state change as ignored

* fixup a35fa82

* Block Editor: Regenerate documentation

aduth added a commit that referenced this pull request Apr 16, 2019

Block Editor: Consider received blocks state change as ignored (#14916)
* Block Editor: Avoid creating new state reference on unchanging isPersistentChange

* Block Editor: Consider received blocks state change as ignored

* fixup a35fa82

* Block Editor: Regenerate documentation

aduth added a commit that referenced this pull request Apr 16, 2019

Block Editor: Consider received blocks state change as ignored (#14916)
* Block Editor: Avoid creating new state reference on unchanging isPersistentChange

* Block Editor: Consider received blocks state change as ignored

* fixup a35fa82

* Block Editor: Regenerate documentation

This was referenced Apr 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.