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

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

Merged
merged 30 commits into from Aug 22, 2019

Conversation

@epiqueras
Copy link
Contributor

commented Aug 6, 2019

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
Copy link
Contributor

left a comment

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

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.

@epiqueras epiqueras force-pushed the update/the-editor-store-to-use-core-data-entities branch 3 times, most recently from 49c7aa8 to 56d0e9e Aug 8, 2019

@epiqueras epiqueras requested review from gziolo and ntwb as code owners Aug 9, 2019

@epiqueras epiqueras force-pushed the update/the-editor-store-to-use-core-data-entities branch from fefcb29 to bfee37a Aug 9, 2019

@epiqueras epiqueras requested a review from ellatrix as a code owner Aug 9, 2019

@gziolo
Copy link
Member

left a comment

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.

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

This comment has been minimized.

Copy link
@gziolo

gziolo Aug 12, 2019

Member

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.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Aug 12, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 13, 2019

Contributor

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.

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

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

This comment has been minimized.

Copy link
@gziolo

gziolo Aug 12, 2019

Member

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?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Aug 12, 2019

Author Contributor

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.

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

This comment has been minimized.

Copy link
@gziolo

gziolo Aug 12, 2019

Member

Why those functions are part of the state?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Aug 12, 2019

Author Contributor

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.

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

This comment has been minimized.

Copy link
@gziolo

gziolo Aug 12, 2019

Member

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.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Aug 12, 2019

Author Contributor

#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
Copy link
Contributor

left a comment

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

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

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

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 13, 2019

Contributor

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

This comment has been minimized.

Copy link
@epiqueras

epiqueras Aug 13, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 14, 2019

Contributor

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

This comment has been minimized.

Copy link
@epiqueras

epiqueras Aug 14, 2019

Author Contributor

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 Outdated Show resolved Hide resolved
packages/core-data/src/selectors.js Outdated Show resolved Hide resolved
@@ -151,6 +151,7 @@ describe( 'Change detection', () => {

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

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 13, 2019

Contributor

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.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Aug 13, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 14, 2019

Contributor

Why? why it was changed?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Aug 14, 2019

Author Contributor

Because:

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

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 15, 2019

Contributor

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

This comment has been minimized.

Copy link
@epiqueras

epiqueras Aug 16, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 16, 2019

Contributor

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.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Aug 16, 2019

Author Contributor

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

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 16, 2019

Contributor

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

This comment has been minimized.

Copy link
@epiqueras

epiqueras Aug 20, 2019

Author Contributor
packages/editor/src/store/actions.js Outdated Show resolved Hide resolved
packages/editor/src/store/actions.js Outdated Show resolved Hide resolved
*
* @return {string} The blocks serialization.
*/
export const serializeBlocks = memoize(

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 13, 2019

Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2019

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.

@epiqueras epiqueras force-pushed the update/the-editor-store-to-use-core-data-entities branch 2 times, most recently from 0ca7b0f to 450b7f2 Aug 15, 2019

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

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

Let me know if I missed anything.

@epiqueras epiqueras force-pushed the update/the-editor-store-to-use-core-data-entities branch from 4d5de1b to a020241 Aug 22, 2019

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

Looks like there's a failing e2e test. (you should rebase to fix the builid artifacts), Other than that, this LGTM. Thanks

Done! 🎉🎉🎉🎉

@epiqueras epiqueras merged commit 4944315 into master Aug 22, 2019

2 checks passed

pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details

@epiqueras epiqueras added this to Done in Phase 2 via automation Aug 22, 2019

donmhico added a commit to donmhico/gutenberg that referenced this pull request Aug 27, 2019
Editor: Update the store to use Core Data entities. (WordPress#16932)
* 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.
dratwas added a commit to callstack/gutenberg that referenced this pull request Aug 28, 2019
Editor: Update the store to use Core Data entities. (WordPress#16932)
* 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.
gziolo added a commit that referenced this pull request Aug 29, 2019
Editor: Update the store to use Core Data entities. (#16932)
* 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

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

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.

@gziolo gziolo deleted the update/the-editor-store-to-use-core-data-entities branch Aug 29, 2019

gziolo added a commit that referenced this pull request Aug 29, 2019
Editor: Update the store to use Core Data entities. (#16932)
* 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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

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

if ( options.__unstableShouldCreateUndoLevel !== false ) {

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

This comment has been minimized.

Copy link
@ellatrix

ellatrix Sep 2, 2019

Member

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

This comment has been minimized.

Copy link
@epiqueras

epiqueras Sep 2, 2019

Author Contributor

Initial edits should be considered a dirtying operation.

See #16932 (comment)

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

await assertIsDirty( false );
await assertIsDirty( true );

This comment has been minimized.

Copy link
@ellatrix

ellatrix Sep 2, 2019

Member

Why has this changed?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Sep 2, 2019

Author Contributor

Initial edits should be considered a dirtying operation.

See #16932 (comment)

@@ -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' );

This comment has been minimized.

Copy link
@ellatrix

ellatrix Sep 2, 2019

Member

What has changed so that this became necessary?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Sep 2, 2019

Author Contributor

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

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

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

This comment has been minimized.

Copy link
@ellatrix

ellatrix Sep 2, 2019

Member

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

This comment has been minimized.

Copy link
@epiqueras

epiqueras Sep 2, 2019

Author Contributor

Initial edits should be considered a dirtying operation.

See #16932 (comment)

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.