Refactor the initialization of the editor, require only a post ID by youknowriad · Pull Request #6384 · WordPress/gutenberg

youknowriad · 2018-04-24T13:17:56Z

Previously to initialize the Gutenberg edit-post module we had to pass a post object to the initializeEditor function. This PR updates this behavior and requires only a postId. A defaultPost is still kept to allow passing intialization attributes (essentially for demo content)

There are several reasons for this:

Making the editor more independent from the backend
Leveraging the data module to fetch the post objects
And more importantly, this prepares the way to move post saving into edit-post (relying on the data module).
It also simplifies a lot of code in client-assets and thanks to the preloading support in wp.apiRequest, the post is still loaded instantaneously.

Testing instructions

Test that you can create a new post
you can open an existing post
you can load the demo content.

youknowriad · 2018-04-24T13:19:33Z

lib/client-assets.php

 		sprintf( '/wp/v2/types/%s?context=edit', $post_type ),
 		sprintf( '/wp/v2/users/me?post_type=%s&context=edit', $post_type ),
 		'/wp/v2/taxonomies?context=edit',
-		gutenberg_get_rest_link( $post_to_edit, 'about', 'edit' ),


Not certain what this call was preloading but I didn't notice any regression in term of preloaded requests, let me know if I'm missing something.

Not certain what this call was preloading but I didn't notice any regression in term of preloaded requests, let me know if I'm missing something.

I guess it was preloading the post type data. Unclear where that was used, but I cannot see anything immediately that should break. Probably safe. I probably should have documented it better in the first place 😬

gziolo · 2018-04-24T13:30:08Z

lib/client-assets.php

 		window._wpLoadGutenbergEditor = wp.api.init().then( function() {
 			wp.blocks.registerCoreBlocks();
-			return wp.editPost.initializeEditor( 'editor', window._wpGutenbergPost, editorSettings );
+			return wp.editPost.initializeEditor( 'editor', window._wpGutenbergPostId, editorSettings, window._wpGutenbergDefaultPost );


Can you simply inline _wpGutenbergPostId and _wpGutenbergDefaultPost?

Not sure what you mean? The reason those are different and not at the same position is that one is mandatory while the other is optional and could probably be removed later if we remove the demo content.

Why not wrap this code with sprintf and inject those 2 variables directly in here instead of using globals?

It's difficult to express with PHP but simplified version would be:

if ( $is_new_post ) { default_post = array( 'title' => ... ); } else { $post_id = $post->id; } $init_script = <<<JS ... return wp.editPost.initializeEditor( 'editor', %s, editorSettings, %s ); ... JS; $script .= sprintf( init_script, $post_id, wp_json_encode( $default_post ) );

Gotcha, window._wpGutenbergDefaultPost can be undefined in some cases but I guess it's feasible.

Ok, this doesn't work easily because of the content of the demo post is a JavaScript file. We could try to convert it to JSON but seems not worth it.

aduth · 2018-04-25T21:06:46Z

edit-post/index.js

+ * @param {Object}  postId   ID of the post to edit.
 * @param {Element} target   DOM node in which editor is rendered.
 * @param {?Object} settings Editor settings object.
+ * @param {Object}  defaultPost   Post initilization object


Nit: Alignment of parameters.

aduth · 2018-04-25T21:07:19Z

lib/client-assets.php


 	// Preload common data.
 	$preload_paths = array(
+		sprintf( '/wp/v2/posts/%s?context=edit', $post->ID ),


Nit: Placeholder could be %d, expecting numeric.

aduth · 2018-04-25T21:12:25Z

lib/client-assets.php

 		sprintf( '/wp/v2/types/%s?context=edit', $post_type ),
 		sprintf( '/wp/v2/users/me?post_type=%s&context=edit', $post_type ),
 		'/wp/v2/taxonomies?context=edit',
-		gutenberg_get_rest_link( $post_to_edit, 'about', 'edit' ),


Not certain what this call was preloading but I didn't notice any regression in term of preloaded requests, let me know if I'm missing something.

I guess it was preloading the post type data. Unclear where that was used, but I cannot see anything immediately that should break. Probably safe. I probably should have documented it better in the first place 😬

aduth · 2018-04-25T21:13:46Z

lib/client-assets.php

 	wp_add_inline_script(
 		'wp-edit-post',
-		'window._wpGutenbergPost = ' . wp_json_encode( $post_to_edit ) . ';'
+		sprintf( 'window._wpGutenbergPostId = %s;', $post->ID )


Should we drop the Gutenberg codename ? 😄

aduth · 2018-04-25T21:13:56Z

lib/client-assets.php

-	) );
+		'post' => $post->ID,
+
+		) );


Odd tabbing.

aduth · 2018-04-25T21:15:11Z

edit-post/editor.js

+
 	return (
-		<EditorProvider settings={ { ...settings, hasFixedToolbar } } { ...props }>
+		<EditorProvider settings={ editorSettings } post={ { ...post, ...defaultPost } } { ...props }>


Shouldn't the spread be the other way around? { ...defaultPost, ...post }

Shouldn't the spread be the other way around? { ...defaultPost, ...post }

Still wondering about this note.

Okay, I see this may be needed because auto-draft title replacement is meant to take precedent over the default title. I wonder if we could...

Avoid needing this override in the first place (set auto-draft title as it's created?)

Rename the variable to reflect that it's an override, not a set of fallback values

Comment to reflect the intended usage

👍 I'll clarify this a bit and agree it would be better if we can remove it entirely but I didn't want to bake this into our modules. I think once we remove the demo post, we'd be able to remove those but not before.

I think it's also needed for the new post, since auto-draft is created with a "(Auto-draft)" title and we don't want to show this in the editor, so we needed to be able to override, though I think ideally we just... don't assign that title 😄

youknowriad · 2018-05-03T09:53:43Z

I updated the PR and added dynamic loading of "entities" given a "kind" in 7e53517

Before, I go further by adding unit tests etc... I'd appreciate some thoughts on the approach here and ideas to improve it.

The idea is the following:

We need to automatically load a post given its id and post type using getEntityRecord( "postType", "post", id )
And this should work for any post or Custom Post Type which means we can't statically load the posts from /wp/v2/posts, we need to map each post type to its rest base.
So what I did in the resolver getEntityRecord I added a mechanism to load the entities associated with a kind (in our example the kind is postType). So what happens is if you call getEntityRecord if the requested entity is not loaded we try to dynamically load the config for all the entities of the given kind.

Thoughs @gziolo @aduth @WordPress/gutenberg-core

aduth

The fact we have loadKindEntities as a utility within resolvers.js has me wondering if we should have it be a proper resolver; i.e. have a reducer which returns the entity configuration for a given post type, and resolve it if necessary, so it's not treated any different than another selector. Would that be possible?

aduth · 2018-05-07T18:38:23Z

core-data/entities.js

-export function getEntity( kind, name ) {
-	return find( entities, { kind, name } );
+async function loadPostTypeEntities() {
+	const postTypes = await apiRequest( { path: '/wp/v2/types?context=edit' } );


Is ?context=edit valid for the types endpoint? I'm seeing an error when trying to request it directly this way.

Weird, It worked for me (I removed the preloading and I do see the request in the network tab)

aduth · 2018-05-07T18:40:11Z

core-data/resolvers.js

 export async function* getEntityRecord( state, kind, name, key ) {
-	const entity = getEntity( kind, name );
+	yield* loadKindEntities( state, kind );
+	// I can't use the state because it's outdated at this point


Could we use the yielded result from loadKindEntities?

The problem is loadKindEntities don't do anything if the entities are loaded. It can be updated to return the entities in all cases. So it can be renamed getKindEntities which could be just a regular resolver for the getKindEntities selector and that's the point where I felt the need of a resolve API.

yield resolve( 'core' ).getKindEntities() or something like that where I won't be obliged to call the selector getKindEntities by myself inside the resolver but it will be called automatically when the resolver finishes.

I guess it's still not clear if it will be a common use-case etc... I prefer to wait a bit with and keep the current "shortcut"

Ok so to simplify unit-testing, I went ahead and returned the available entities in all use-cases.

aduth · 2018-05-07T18:49:53Z

core-data/resolvers.js

 }

+async function* loadKindEntities( state, kind ) {
+	const hasEntities = hasEntitiesByKind( state, kind );


What is this doing?

The idea was to avoid loading the entities again once already loaded.

aduth · 2018-05-07T18:50:22Z

core-data/selectors.js

+ * @return {boolean} Whether the entities are loaded
+ */
+export function hasEntitiesByKind( state, kind ) {
+	return state.entities.config[ kind ] !== undefined;


entities.config is an array? When will this ever not be false ?

I guess this is broken :)

aduth · 2018-05-07T18:54:51Z

core-data/reducer.js

+	const newConfig = entitiesConfig( state.config, action );
+
+	// Generates a dynamic reducer for the entities
+	const entitiesByKind = groupBy( newConfig, 'kind' );


Does this need to be run on every dispatch? Or could we limit it to those which would actually impact the configuration to justify dynamically creating new reducers?

Do you I should use a local variable to keep track of the current reducer?

Maybe it's included as part of the state value?

It feels a bit weird to store the reducer in state though :). I don't have a strong preference though, I'll try this.

It does seem strange, though accurate if we're saying that the reducer itself is dependent on state, that it is itself state.

Or, if it's a derivation, then a selector may be appropriate?

Entering unfamiliar territory for me with this dynamic reducer 😄

aduth · 2018-05-07T18:55:17Z

core-data/reducer.js

+ *
+ * @return {Object} Updated state.
+ */
+export function posts( state = {}, action ) {


Do we want this reducer? Isn't this what entities is meant to be providing on our behalf?

aduth · 2018-05-07T18:58:38Z

core-data/resolvers.js

 	yield receiveUserQuery( 'authors', users );
 }

+async function* loadKindEntities( state, kind ) {


This isn't so much a resolver, is it? More a utility?

Well, the difference is not totally clear here since it uses selectors and yield actions.
Anyway, I moved it to the entities.js file for now.

youknowriad · 2018-05-08T12:54:07Z

Let me know if you'll agree with the ideas here so I can move forward with the testing etc...

Also, I'm looking forward using the data module to extract saving posts from the editor module into the edit-post module.

aduth · 2018-05-14T14:38:33Z

core-data/reducer.js

+	const newConfig = entitiesConfig( state.config, action );
+
+	// Generates a dynamic reducer for the entities
+	const entitiesByKind = groupBy( newConfig, 'kind' );


Maybe it's included as part of the state value?

aduth · 2018-05-14T14:40:17Z

edit-post/editor.js

+
 	return (
-		<EditorProvider settings={ { ...settings, hasFixedToolbar } } { ...props }>
+		<EditorProvider settings={ editorSettings } post={ { ...post, ...defaultPost } } { ...props }>


Shouldn't the spread be the other way around? { ...defaultPost, ...post }

Still wondering about this note.

aduth · 2018-05-14T14:47:28Z

edit-post/editor.js

+
 	return (
-		<EditorProvider settings={ { ...settings, hasFixedToolbar } } { ...props }>
+		<EditorProvider settings={ editorSettings } post={ { ...post, ...defaultPost } } { ...props }>


Okay, I see this may be needed because auto-draft title replacement is meant to take precedent over the default title. I wonder if we could...

Avoid needing this override in the first place (set auto-draft title as it's created?)

Rename the variable to reflect that it's an override, not a set of fallback values

Comment to reflect the intended usage

aduth · 2018-05-14T15:02:41Z

core-data/entities.js

+	entities = await kindConfig.loadEntities();
+	yield addEntities( entities );
+
+	return entities;


The multipurposing of this function feels a bit awkward / forced. Not sure I have a great solution here. One option is to make it always yield the action, and extract this from the getEntityRecord resolver:

export async function* getKindEntities( state, kind ) { const kindConfig = find( kinds, { name: kind } ); if ( ! kindConfig ) { return; } let entities = getEntitiesByKind( state, kind ); if ( ! entities ) { entities = await kindConfig.loadEntities(); } yield addEntities( entities ); }

export async function* getEntityRecord( state, kind, name, key ) { const { entities } = await ( await getKindEntities( state, kind ).next() ).value; const entity = find( entities, { kind, name } ); if ( ! entity ) { return; } const record = await apiRequest( { path: `${ entity.baseUrl }/${ key }?context=edit` } ); yield receiveEntityRecords( kind, name, record ); }

This unfortunately causes getEntityRecord to have a strong awareness of what's yielded by getKindEntities, which could break down if ever getKindEntities yields more or different than just the action of ADD_ENTITIES. We could make getKindEntities not a generator, which could simplify it a bit more.

In retrospect, I might be more open to the idea of using select from within the resolver as was originally proposed, if it avoids some of the complexity here.

I do agree, it's a bit weird, I'd like us to avoid adding complexity for the moment with a new API or something, let's try it for some time. We'll see how it plays when adding other entity selectors. I'm kind of hesitant to introduce select within a resolver just for the entities use-case.

youknowriad · 2018-05-24T10:13:57Z

Rebased this, it's ready for a final review

aduth

The entities implementation is becoming a bit hard to follow in places, while simultaneously impressive in its coordinated result. I think this is an important refactoring, so would be inclined to move forward on it and iterate as necessary on entities in future iterations. Nice work 👍

(Needs a rebase)

aduth · 2018-05-25T14:09:22Z

core-data/actions.js

 		themeSupports: index.theme_supports,
 	};
 }
+


Nit: Unnecessary newline.

aduth · 2018-05-25T14:11:55Z

core-data/entities.js

+ * Returns the list of post type entities.
 *
- * @return {Object} Entity
+ * @return {Array} entities


Pedantry: An async function always returns type Promise†. In this case it's a promise which resolves with an array.

† ...which ESLint makes quite annoying for its valid-jsdoc rule since it assumes the presence of a return statement, which isn't always the case:

async function sleep( duration ) { return new Promise( ( resolve ) => { setTimeout( resolve, duration ); } ); } async function resolve() { await sleep( 1000 ); await sleep( 1000 ); } console.log( resolve() instanceof Promise ); // true

aduth · 2018-05-25T14:24:32Z

core-data/resolvers.js

 */
 export async function* getEntityRecord( state, kind, name, key ) {
-	const entity = getEntity( kind, name );
+	const entities = yield* getKindEntities( state, kind );


I've reached the extent of my knowledge of asynchronous generators 😄 Is an await keyword needed on this line if getKindEntities may only return (resolve) after an async apiRequest ?

aduth · 2018-05-25T14:31:06Z

edit-post/index.js

 * @return {Object} Editor interface.
 */
-export function initializeEditor( id, post, settings ) {
+export function initializeEditor( id, postId, postType, settings, overridePost ) {


In a potential future where postId becomes optional, would we want to support calling this as simply...

initializeEditor( 'gutenberg', 'post' );

i.e. with postType before the optional postId

Asides:

id is pretty useless now. It was originally intended for separate instances of an editor, which we don't really support. Maybe it should just be dropped.

At what point might we consider an object argument? Since we could have many variations (post with ID and no overrides, post with no ID and overrides, post with overrides but no settings)

I'll keep the id for now. I'm not certain if we'll support multi-instances at some point but prefer to keep it for now.

An object argument: I don't know 🤷‍♂️, It may feel like a component though and maybe keeping it as clarify the distinction?

youknowriad · 2018-05-28T12:03:16Z

I rebase this PR and tests are failing. The problem is that the REST API hooks adding action-publish flag are not being used when preloading data. I believe this is a "hooks" order issue. @danielbachhuber Any idea how to fix that? What hook should I use instead of registered_post_type? Any other idea?

danielbachhuber · 2018-05-29T13:00:11Z

The problem is that the REST API hooks adding action-publish flag are not being used when preloading data.

Are you sure about this? I'm not able to reproduce the e2e test failure locally.

This doesn't seem like a load order issue to me. registered_post_type is much earlier than admin_enqueue_scripts.

youknowriad · 2018-05-29T13:20:55Z

@danielbachhuber In my testing yesterday the post object in the preloaded data didn't have the "action-publish". I'll try again in a bit to confirm.

…tead of a post object

youknowriad · 2018-06-05T10:24:33Z

Ok so it looks like our preloading function was not including the _links into the response. It should be fied with e743830

youknowriad self-assigned this Apr 24, 2018

youknowriad requested a review from aduth April 24, 2018 13:17

youknowriad commented Apr 24, 2018

View reviewed changes

gziolo reviewed Apr 24, 2018

View reviewed changes

youknowriad force-pushed the refactor/editor-initialization branch 2 times, most recently from dfc8319 to b649a29 Compare April 25, 2018 09:12

aduth reviewed Apr 25, 2018

View reviewed changes

youknowriad mentioned this pull request Apr 27, 2018

Initial entities implementation #6462

Merged

youknowriad force-pushed the refactor/editor-initialization branch from 8f2a8a9 to f776128 Compare May 3, 2018 09:48

youknowriad added the Framework Issues related to broader framework topics, especially as it relates to javascript label May 3, 2018

youknowriad force-pushed the refactor/editor-initialization branch from f776128 to 7e53517 Compare May 3, 2018 09:55

aduth reviewed May 7, 2018

View reviewed changes

youknowriad force-pushed the refactor/editor-initialization branch from 7e53517 to a3f1ddf Compare May 8, 2018 12:23

youknowriad force-pushed the refactor/editor-initialization branch from cd85917 to dcb64f1 Compare May 10, 2018 14:52

aduth reviewed May 14, 2018

View reviewed changes

youknowriad force-pushed the refactor/editor-initialization branch from cc723c2 to 4576a31 Compare May 17, 2018 08:01

youknowriad force-pushed the refactor/editor-initialization branch from a2c368a to 6debf9e Compare May 24, 2018 10:13

aduth mentioned this pull request May 25, 2018

Feature: Markdown support as a Jetpack Markdown-specific Gutenberg Block #6491

Closed

5 tasks

aduth approved these changes May 25, 2018

View reviewed changes

youknowriad force-pushed the refactor/editor-initialization branch 2 times, most recently from 8ae266f to a25674f Compare May 28, 2018 11:44

youknowriad added 3 commits June 5, 2018 10:43

Refactor the initialization of the editor, require only a post ID ins…

7f0177a

…tead of a post object

Fix phpcs tests

059cde3

Load dynamically entities by kind

28be8f0

youknowriad added 12 commits June 5, 2018 10:43

Fix selector hasEntitiesByKind

d37783d

Remove useless posts reducer

aef4ada

Move LoadKindEntities to the entities file

b69f5c1

Add unit tests

5b1076e

Fix unit tests

f2d4a87

Fix custom post types preloading

7127cc0

Avoid generating a new reducer on each call if the config didn't change

2eb7f16

Rename defaultPost => overridePost` to clarify its meaning

d29c910

Fix unit tests

66b44bd

Fix unit tests after rebase

8a067ed

Adding await when calling async generators

91f36ab

Switch initializeEditor arguments

48425b2

youknowriad force-pushed the refactor/editor-initialization branch from a25674f to 48425b2 Compare June 5, 2018 09:47

youknowriad added 2 commits June 5, 2018 10:58

Fix unit tests

5f2b411

Returning the _links from the preloaded endpoints

e743830

Code style fixes

ab1e150

youknowriad merged commit 7c149d7 into master Jun 6, 2018

youknowriad deleted the refactor/editor-initialization branch June 6, 2018 08:50

youknowriad added this to the 3.1 milestone Jun 6, 2018

aduth mentioned this pull request Jun 6, 2018

REST API: Autosaves entities should include preview_link #7179

Closed

aduth mentioned this pull request Jun 26, 2018

Attempts to edit posts that contain Strong Testimonials plugin shortcodes open as blank pages #7461

Closed

Conversation

youknowriad commented Apr 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad commented May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

gziolo Apr 24, 2018 •

edited

Loading

gziolo Apr 24, 2018 •

edited

Loading

youknowriad commented May 3, 2018 •

edited

Loading

aduth May 14, 2018 •

edited

Loading