Editor: Update the store to use Core Data entities. #16932

epiqueras · 2019-08-06T21:37:37Z

Description

This PR picks up from where #16903 left off in the planned entities refactor. It almost completely rewrites the editor store to act as a thin routing interface between the Block Editor and Core Data entities, in preparation for multi-entity editor setups for full site editing.

How has this been tested?

The existing tests were refactored to work with the new code.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.

youknowriad

Quickly looking at the code, it looks great. I love how this simplifies a lot of things. The different responsibilities become way clearer.

Do you know why the tests are failing? I feel like tests failing in big refactorings can have a lot of impact so I'd prefer to wait before doing a deeper review.

epiqueras · 2019-08-07T14:44:33Z

Do you know why the tests are failing? I feel like tests failing in big refactorings can have a lot of impact so I'd prefer to wait before doing a deeper review.

No idea, but it looks pretty scary. I'll see what's going on and ping you when I know more.

gziolo

It's great to see this refactoring happening. We will benefit from it immensely in the long run.

I have some concerns with regards to unit tests. They are essential for this part of the application in the context of maintenance. This is a very complex code and only a good set of unit and e2e tests can help us ensure it's easier to update for a wider group of contributors. In some places, I'm not 100% we that removed tests shouldn't be replaced with their corresponding version translated to the new conditions. However, with the huge number of lines updated in this PR it's hard to tell whether I make correct assumptions.

/cc @hypest and @koke - it feels like this PR should be validated against mobile app to ensure it doesn't break there.

gziolo · 2019-08-12T07:00:47Z

packages/editor/src/store/actions.js

+ *
+ * @return {string} The blocks serialization.
+ */
+export const serializeBlocks = memoize(


Should this helper be exposed as part of the public API?

There are no corresponding unit tests so it doesn't seem like it's used outside of this file.

It's used in tests and a selector as a fallback. It gets tested indirectly in a lot of places that match the post content.

Anything exported in this file gets exported as a store action. It could be moved to a utils file if needed or something like that.

gziolo · 2019-08-12T07:38:38Z

packages/editor/src/store/test/selectors.js

 import { PREFERENCES_DEFAULTS } from '../defaults';
 import { POST_UPDATE_TRANSACTION_ID } from '../constants';

+const selectors = { ..._selectors };
+const selectorNames = Object.keys( selectors );
+selectorNames.forEach( ( name ) => {


Looking at this custom logic which replaces the production code, I'm wondering whether all the tests in the current form are still valuable. I don't think that having 100-ish lines of complex logic is something we want to maintain in the long run.

Have you considered any alternatives? Why did you decide to take this approach?

It's the same registry selector mocking that was being done for isEditedPostAutosaveable, but in a general way without having to inline it into every test case.

gziolo · 2019-08-12T07:41:05Z

packages/editor/src/store/test/selectors.js

@@ -1334,20 +1382,21 @@ describe( 'selectors', () => {
 				saving: {
 					requesting: true,
 				},
+				getCurrentUser() {},


Why those functions are part of the state?

It's part of the registry selector mocking done above. These parts don't have a legacy editor store counterpart like saving does. So, I just inlined a mock getter instead of making up a piece of state which could be confusing. This sort of "remembers" that difference.

gziolo · 2019-08-12T07:43:02Z

packages/editor/src/store/test/selectors.js

@@ -1362,25 +1411,17 @@ describe( 'selectors', () => {
 					title: 'sassel',
 				},
 				saving: {},
+				getCurrentUser() {},


It looks like those functions are added here only to satisfy custom implementation of selectors. I think we should explore some alternatives to make those tests easier to read.

#16932 (comment)

I agree it makes the tests harder to reason about at first glance. But, I think it's a fine trade-off against rewriting all of them in this PR without really gaining extra coverage or a better harness.

youknowriad

nice work here. Nice to see all this removed code.

We'd have to run some performance tests on this PR

youknowriad · 2019-08-13T08:55:08Z

packages/block-editor/src/components/block-list/block.js

+				blocks.length &&
+				! isUnmodifiedDefaultBlock( blocks[ blocks.length - 1 ] )
+			) {
+				__unstableMarkLastChangeAsPersistent();


Any reason for this change? I wouldn't have expected this refactoring to touch the components directly?

We need those replacements to be persistent so that undo works as expected in the E2E tests.

I mean what's changed? Why do we need this to make them persistent? How was it before?

To be honest, I'm surprised it ever worked for this test case specifically:

it( 'can undo asterisk transform', async () => { await clickBlockAppender(); await page.keyboard.type( '1. ' ); await pressKeyWithModifier( 'primary', 'z' ); expect( await getEditedPostContent() ).toMatchSnapshot(); } );

If you do it at a normal pace, it works as expected and undos back to "1. ". But, if you do it super fast, the block editor never marks the last change before the transform as persistent and it goes back to "1". Could something be making this faster so now we can't rely on the time out trigerred __unstableMarkLastChangeAsPersistent?

packages/core-data/src/actions.js

packages/core-data/src/selectors.js

youknowriad · 2019-08-13T09:10:00Z

packages/e2e-tests/specs/change-detection.test.js

@@ -151,6 +151,7 @@ describe( 'Change detection', () => {

 	it( 'Should prompt if content added without save', async () => {
 		await clickBlockAppender();
+		await page.keyboard.type( 'Paragraph' );


This is changing the meaning of the test. The test was saying that if you just click the appender, you don't have to type to consider the post dirty.

Yeah, but an empty paragraph block does not make the post dirty anymore. Other blocks do though.

Why? why it was changed?

Because:

// A single unmodified default block is assumed to // be equivalent to an empty post. if ( blocksForSerialization.length === 1 && isUnmodifiedDefaultBlock( blocksForSerialization[ 0 ] ) ) { blocksForSerialization = []; }

Still, why it wasn't working this way before? Why are we making this User Experience change in this refactoring PR?

This isn't what is forcing us to serialize blocks. We serialize blocks because .content expects a string, not an array of blocks.

Serialization should be even faster than rendering the block list, because save components are generally simpler than edit components, and we are not manipulating a DOM.

There is always the possibility of tagging blocks in core in a way that tells it to do something with it when saving, that something being serializing it and setting content to the result. But, it feels very hacky and we would also need to do it when comparing edits with persisted values.

It was an early decision to not serialize blocks on each change because it's too expensive. (You can probably verify quickly by running the performance tests on this branch).

edit is not expensive because React doesn't rerender everything on each change and with our asyncmode we only rerender synchronously the edited block.

(It only happens on every persistent/undo-level-creating change.)

This branch

Average time to type character: 78.965ms
Slowest time to type character: 170ms
Fastest time to type character: 69ms

master

Average time to type character: 52.535ms
Slowest time to type character: 122ms
Fastest time to type character: 43ms

packages/e2e-tests/specs/change-detection.test.js

packages/editor/src/store/actions.js

youknowriad · 2019-08-13T09:23:07Z

packages/editor/src/store/actions.js

+ *
+ * @return {string} The blocks serialization.
+ */
+export const serializeBlocks = memoize(


Anything exported in this file gets exported as a store action. It could be moved to a utils file if needed or something like that.

epiqueras · 2019-08-13T18:27:09Z

We'd have to run some performance tests on this PR

Definitely.

Anything exported in this file gets exported as a store action. It could be moved to a utils file if needed or something like that.

Yeah good idea, I'll do that now.

packages/core-data/src/selectors.js

packages/editor/src/store/selectors.js

epiqueras · 2019-08-20T01:41:36Z

@youknowriad I implemented everything we talked about and made Travis happy ✅ again.

Let me know if I missed anything.

packages/editor/src/store/actions.js

packages/editor/src/store/selectors.js

* Editor: Update the store to use Core Data entities. * Editor: Fix selector test suites. * Editor: Fix some legacy selectors and behaviors. * Editor: Fix action tests. * Editor: Fix remaining broken unit tests. * Editor: Fix more tests. * Editor: Fix more e2e test behaviors. * Editor: Fix preview functionality. * Core Data: Fix autosaves filtering. * Editor: Don't make entity dirty with initial edits. * Editor: Don't save if the post is not saveable. * Core Data: Fix merged edits logic. * Core Data: Fix undo to fit e2e expected behaviors. * Core Data: Handle more change detection and saving flows. * Block Editor: Fix undo level logic. * Core Data: Clean up undo reducer comment. * Editor: Make `serializeBlocks` a util. * Core Data: Clarify raw attribute usage. * Core Data: Memoize . * Core Data: Use new raw entity record selector instead of modifying the existing one. * Core Data: Make save notices the caller's responsibility. * Editor: Use the store key constant in actions instead of a string literal. * Editor: Defer serialization of blocks until save. * Editor: Fix raw content access in set up. * Editor: Revert broken test change. * Editor: Make initial edits a dirtying operation. * Editor: Add comment clarifying why we set content to a new function on edits. * Demo: Fix tests to consider the initial edits dirtying. * Core Data: Set auto-drafts to drafts when autosaving them. * Core Data: Handle receiving autosaves correctly when editing non-autosave-persisting-properties.

ellatrix · 2019-08-29T11:44:49Z

I see some regressions after this PR, after a quick round of testing. Not sure if it's all because of this PR.

If I create a new post, or a post with the demo content, the undo button is active. I thought we had an e2e test for this, I'm not sure now.
When I type, stop for a second, type again, I need to press the undo button twice to undo the first typing.
When I type, the undo button doesn't immediately become active.
When I update a post, an undo level gets added. Generally, it looks like too many undo levels get added.

If feel like we should have added more undo e2e tests before rewriting.

* Editor: Update the store to use Core Data entities. * Editor: Fix selector test suites. * Editor: Fix some legacy selectors and behaviors. * Editor: Fix action tests. * Editor: Fix remaining broken unit tests. * Editor: Fix more tests. * Editor: Fix more e2e test behaviors. * Editor: Fix preview functionality. * Core Data: Fix autosaves filtering. * Editor: Don't make entity dirty with initial edits. * Editor: Don't save if the post is not saveable. * Core Data: Fix merged edits logic. * Core Data: Fix undo to fit e2e expected behaviors. * Core Data: Handle more change detection and saving flows. * Block Editor: Fix undo level logic. * Core Data: Clean up undo reducer comment. * Editor: Make `serializeBlocks` a util. * Core Data: Clarify raw attribute usage. * Core Data: Memoize . * Core Data: Use new raw entity record selector instead of modifying the existing one. * Core Data: Make save notices the caller's responsibility. * Editor: Use the store key constant in actions instead of a string literal. * Editor: Defer serialization of blocks until save. * Editor: Fix raw content access in set up. * Editor: Revert broken test change. * Editor: Make initial edits a dirtying operation. * Editor: Add comment clarifying why we set content to a new function on edits. * Demo: Fix tests to consider the initial edits dirtying. * Core Data: Set auto-drafts to drafts when autosaving them. * Core Data: Handle receiving autosaves correctly when editing non-autosave-persisting-properties.

epiqueras · 2019-08-29T15:13:25Z

Undo levels get created whenever the block editor calls onInput, here:

gutenberg/packages/editor/src/store/actions.js

Line 818 in 02c5f39

if ( options.__unstableShouldCreateUndoLevel !== false ) {

ellatrix · 2019-09-02T09:29:05Z

packages/e2e-tests/specs/demo.test.js

 		const isDirty = await page.evaluate( () => {
 			const { select } = window.wp.data;
 			return select( 'core/editor' ).isEditedPostDirty();
 		} );
-		expect( isDirty ).toBeFalsy();


@epiqueras @youknowriad @mcsf Why has this been changed???

Initial edits should be considered a dirtying operation.

See #16932 (comment)

ellatrix · 2019-09-02T12:48:03Z

packages/e2e-tests/specs/change-detection.test.js

 		await createNewPost( {
 			title: 'My New Post',
 			content: 'My content',
 			excerpt: 'My excerpt',
 		} );

-		await assertIsDirty( false );
+		await assertIsDirty( true );


Why has this changed?

Initial edits should be considered a dirtying operation.

See #16932 (comment)

ellatrix · 2019-09-02T13:00:53Z

packages/e2e-tests/specs/change-detection.test.js

@@ -240,6 +241,7 @@ describe( 'Change detection', () => {
 		await pressKeyWithModifier( 'primary', 'S' );

 		await page.type( '.editor-post-title__input', '!' );
+		await page.waitForSelector( '.editor-post-save-draft' );


What has changed so that this became necessary?

It wasn't deterministically waiting enough time. I think it might have relied on a race condition.

ellatrix · 2019-09-02T13:02:57Z

packages/editor/src/store/test/selectors.js

@@ -1609,7 +1646,7 @@ describe( 'selectors', () => {
 				currentPost: {},
 			};

-			expect( isEditedPostEmpty( state ) ).toBe( false );
+			expect( isEditedPostEmpty( state ) ).toBe( true );


Why did this change? Why are tests changed without explanation?

Initial edits should be considered a dirtying operation.

See #16932 (comment)

aduth · 2019-12-05T22:38:37Z

packages/core-data/src/selectors.js

+		const record = getEntityRecord( state, kind, name, key );
+		return (
+			record &&
+							Object.keys( record ).reduce( ( acc, _key ) => {


Tabbing is a little wild here. Not sure why ESLint didn't catch this.

epiqueras added [Package] Core data /packages/core-data [Package] Editor /packages/editor labels Aug 6, 2019

epiqueras added this to the Future milestone Aug 6, 2019

epiqueras requested review from ajitbohra, chrisvanpatten, mkaz, nerrad, talldan and youknowriad as code owners August 6, 2019 21:37

epiqueras self-assigned this Aug 6, 2019

youknowriad reviewed Aug 7, 2019

View reviewed changes

epiqueras force-pushed the update/the-editor-store-to-use-core-data-entities branch 3 times, most recently from 49c7aa8 to 56d0e9e Compare August 8, 2019 23:46

epiqueras requested review from gziolo and ntwb as code owners August 9, 2019 18:26

epiqueras force-pushed the update/the-editor-store-to-use-core-data-entities branch from fefcb29 to bfee37a Compare August 9, 2019 19:47

epiqueras requested a review from ellatrix as a code owner August 9, 2019 19:47

gziolo reviewed Aug 12, 2019

View reviewed changes

epiqueras mentioned this pull request Aug 13, 2019

Editor: Implement the EntityHandlers component and basic template blocks. #17020

Closed

5 tasks

youknowriad reviewed Aug 13, 2019

View reviewed changes

youknowriad reviewed Aug 15, 2019

View reviewed changes

packages/core-data/src/selectors.js Outdated Show resolved Hide resolved

epiqueras force-pushed the update/the-editor-store-to-use-core-data-entities branch 2 times, most recently from 0ca7b0f to 450b7f2 Compare August 19, 2019 18:13

youknowriad reviewed Aug 19, 2019

View reviewed changes

packages/editor/src/store/selectors.js Show resolved Hide resolved

youknowriad reviewed Aug 20, 2019

View reviewed changes

packages/editor/src/store/actions.js Show resolved Hide resolved

youknowriad reviewed Aug 20, 2019

View reviewed changes

packages/editor/src/store/selectors.js Show resolved Hide resolved

pinarol mentioned this pull request Aug 23, 2019

Fix keyboard hide button wordpress-mobile/gutenberg-mobile#1321

Closed

mmtr mentioned this pull request Aug 27, 2019

Private posts/pages triggers unsaved changes warnings #17209

Closed

pinarol mentioned this pull request Aug 27, 2019

Crash on the latest master on gutenberg repo wordpress-mobile/gutenberg-mobile#1332

Closed

gziolo deleted the update/the-editor-store-to-use-core-data-entities branch August 29, 2019 11:48

This was referenced Aug 29, 2019

Record selection changes in history (undo) #16428

Closed

Block Editor: Fix undo level inconsistencies. #17259

Merged

swissspidy mentioned this pull request Aug 30, 2019

Update block editor package ampproject/amp-wp#3149

Merged

ellatrix reviewed Sep 2, 2019

View reviewed changes

epiqueras mentioned this pull request Sep 7, 2019

Overview: Custom Entity Sources #17368

Closed

afercia mentioned this pull request Sep 13, 2019

"Move to Trash" fails to redirect to posts page #17426

Closed

ellatrix mentioned this pull request Oct 7, 2019

Record selection changes in history (undo) #17824

Merged

5 tasks

youknowriad added the Full Site Editing label Oct 9, 2019

mcsf mentioned this pull request Oct 9, 2019

WP 5.3b2 / GB 6.6: changing + saving same inspector setting (diff. values) twice is no longer possible #17852

Closed

gziolo mentioned this pull request Oct 20, 2019

Editor: Refactor currentPost state to use core-data package #16520

Closed

This was referenced Nov 4, 2019

Editor: Save post after deletion so that its status is refreshed and redirection happens. #17427

Merged

Editor: Leverage core data entities for edits, history, change detection #16761

Closed

aduth reviewed Dec 5, 2019

View reviewed changes

aduth mentioned this pull request Dec 5, 2019

Core Data: Add support for fetching permissions of custom actions #18956

Closed

jsnajdr mentioned this pull request Oct 19, 2022

hasChangedContent: remove obsolete blocks check #45090

Merged

Editor: Update the store to use Core Data entities. #16932

Editor: Update the store to use Core Data entities. #16932

Conversation

epiqueras commented Aug 6, 2019 • edited Loading

Description

How has this been tested?

Checklist:

youknowriad left a comment

Choose a reason for hiding this comment

epiqueras commented Aug 7, 2019

gziolo left a comment • edited by koke Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epiqueras commented Aug 13, 2019

epiqueras commented Aug 20, 2019

ellatrix commented Aug 29, 2019

epiqueras commented Aug 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epiqueras commented Aug 6, 2019 •

edited

Loading

gziolo left a comment •

edited by koke

Loading