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
Refactor the initialization of the editor, require only a post ID #6384
Conversation
lib/client-assets.php
Outdated
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' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😬
lib/client-assets.php
Outdated
@@ -966,7 +895,7 @@ function gutenberg_editor_scripts_and_styles( $hook ) { | |||
$script .= <<<JS | |||
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 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you simply inline _wpGutenbergPostId
and _wpGutenbergDefaultPost
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not wrap this code with sprintf
and inject those 2 variables directly in here instead of using globals?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ) );
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, window._wpGutenbergDefaultPost
can be undefined in some cases but I guess it's feasible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
dfc8319
to
b649a29
Compare
edit-post/index.js
Outdated
* @param {Element} target DOM node in which editor is rendered. | ||
* @param {?Object} settings Editor settings object. | ||
* @param {Object} defaultPost Post initilization object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Alignment of parameters.
lib/client-assets.php
Outdated
|
||
// Set the post type name. | ||
$post_type = get_post_type( $post ); | ||
|
||
// Preload common data. | ||
$preload_paths = array( | ||
sprintf( '/wp/v2/posts/%s?context=edit', $post->ID ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Placeholder could be %d
, expecting numeric.
lib/client-assets.php
Outdated
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' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😬
lib/client-assets.php
Outdated
wp_add_inline_script( | ||
'wp-edit-post', | ||
'window._wpGutenbergPost = ' . wp_json_encode( $post_to_edit ) . ';' | ||
sprintf( 'window._wpGutenbergPostId = %s;', $post->ID ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we drop the Gutenberg codename ? 😄
lib/client-assets.php
Outdated
) ); | ||
'post' => $post->ID, | ||
|
||
) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Odd tabbing.
edit-post/editor.js
Outdated
return ( | ||
<EditorProvider settings={ { ...settings, hasFixedToolbar } } { ...props }> | ||
<EditorProvider settings={ editorSettings } post={ { ...post, ...defaultPost } } { ...props }> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the spread be the other way around? { ...defaultPost, ...post }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the spread be the other way around? { ...defaultPost, ...post }
Still wondering about this note.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😄
8f2a8a9
to
f776128
Compare
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:
|
f776128
to
7e53517
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
export function getEntity( kind, name ) { | ||
return find( entities, { kind, name } ); | ||
async function loadPostTypeEntities() { | ||
const postTypes = await apiRequest( { path: '/wp/v2/types?context=edit' } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is ?context=edit
valid for the types endpoint? I'm seeing an error when trying to request it directly this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird, It worked for me (I removed the preloading and I do see the request in the network tab)
core-data/resolvers.js
Outdated
* | ||
* @param {Object} state State tree | ||
* @param {string} kind Entity kind. | ||
* @param {string} name Entity name. | ||
* @param {number} key Record's key | ||
*/ | ||
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use the yielded result from loadKindEntities
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so to simplify unit-testing, I went ahead and returned the available entities in all use-cases.
core-data/resolvers.js
Outdated
@@ -33,16 +43,37 @@ export async function* getAuthors() { | |||
yield receiveUserQuery( 'authors', users ); | |||
} | |||
|
|||
async function* loadKindEntities( state, kind ) { | |||
const hasEntities = hasEntitiesByKind( state, kind ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this doing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was to avoid loading the entities again once already loaded.
core-data/selectors.js
Outdated
* @return {boolean} Whether the entities are loaded | ||
*/ | ||
export function hasEntitiesByKind( state, kind ) { | ||
return state.entities.config[ kind ] !== undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
entities.config
is an array? When will this ever not be false
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is broken :)
core-data/reducer.js
Outdated
const newConfig = entitiesConfig( state.config, action ); | ||
|
||
// Generates a dynamic reducer for the entities | ||
const entitiesByKind = groupBy( newConfig, 'kind' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you I should use a local variable to keep track of the current reducer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's included as part of the state
value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels a bit weird to store the reducer in state though :). I don't have a strong preference though, I'll try this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😄
core-data/reducer.js
Outdated
* | ||
* @return {Object} Updated state. | ||
*/ | ||
export function posts( state = {}, action ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want this reducer? Isn't this what entities is meant to be providing on our behalf?
core-data/resolvers.js
Outdated
@@ -33,16 +43,37 @@ export async function* getAuthors() { | |||
yield receiveUserQuery( 'authors', users ); | |||
} | |||
|
|||
async function* loadKindEntities( state, kind ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't so much a resolver, is it? More a utility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
7e53517
to
a3f1ddf
Compare
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 |
cd85917
to
dcb64f1
Compare
core-data/reducer.js
Outdated
const newConfig = entitiesConfig( state.config, action ); | ||
|
||
// Generates a dynamic reducer for the entities | ||
const entitiesByKind = groupBy( newConfig, 'kind' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's included as part of the state
value?
edit-post/editor.js
Outdated
return ( | ||
<EditorProvider settings={ { ...settings, hasFixedToolbar } } { ...props }> | ||
<EditorProvider settings={ editorSettings } post={ { ...post, ...defaultPost } } { ...props }> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the spread be the other way around? { ...defaultPost, ...post }
Still wondering about this note.
edit-post/editor.js
Outdated
return ( | ||
<EditorProvider settings={ { ...settings, hasFixedToolbar } } { ...props }> | ||
<EditorProvider settings={ editorSettings } post={ { ...post, ...defaultPost } } { ...props }> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
entities = await kindConfig.loadEntities(); | ||
yield addEntities( entities ); | ||
|
||
return entities; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
cc723c2
to
4576a31
Compare
a2c368a
to
6debf9e
Compare
Rebased this, it's ready for a final review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
core-data/actions.js
Outdated
@@ -81,3 +95,4 @@ export function receiveThemeSupportsFromIndex( index ) { | |||
themeSupports: index.theme_supports, | |||
}; | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Unnecessary newline.
core-data/entities.js
Outdated
* | ||
* @return {Object} Entity | ||
* @return {Array} entities |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
core-data/resolvers.js
Outdated
* | ||
* @param {Object} state State tree | ||
* @param {string} kind Entity kind. | ||
* @param {string} name Entity name. | ||
* @param {number} key Record's key | ||
*/ | ||
export async function* getEntityRecord( state, kind, name, key ) { | ||
const entity = getEntity( kind, name ); | ||
const entities = yield* getKindEntities( state, kind ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
edit-post/index.js
Outdated
* | ||
* @return {Object} Editor interface. | ||
*/ | ||
export function initializeEditor( id, post, settings ) { | ||
export function initializeEditor( id, postId, postType, settings, overridePost ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
8ae266f
to
a25674f
Compare
I rebase this PR and tests are failing. The problem is that the REST API hooks adding |
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. |
@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. |
a25674f
to
48425b2
Compare
Ok so it looks like our preloading function was not including the |
Previously to initialize the Gutenberg
edit-post
module we had to pass a post object to theinitializeEditor
function. This PR updates this behavior and requires only apostId
. AdefaultPost
is still kept to allow passing intialization attributes (essentially for demo content)There are several reasons for this:
edit-post
(relying on thedata
module).client-assets
and thanks to the preloading support inwp.apiRequest
, the post is still loaded instantaneously.Testing instructions