[Navigation screen] Replace component state and refs with Redux state #23033
Conversation
|
Size Change: +1.08 kB (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
| menuItems | ||
| ); | ||
| const saveMenuItems = () => eventuallySaveMenuItems( blocks, menuItemsRef ); | ||
| const postId = useStubPost( query ); |
There was a problem hiding this comment.
Instead of keeping blocks in state, this PR experiments with creating a "stub" post that serves as container for all our blocks but is never meant to be saved.
| const [ | ||
| blocks, | ||
| onInput, | ||
| onChange, | ||
| saveMenuItems, | ||
| ] = useNavigationBlockEditor( query, postId ); |
There was a problem hiding this comment.
Instead of having custom block/onInput/onChange handlers, this uses the same ones as the regular post editor.
| halt() { | ||
| this.halted = true; | ||
| this.queue = []; | ||
| this.listeners = []; |
There was a problem hiding this comment.
Remember how new menu items are created whenever a new block is added? Changing an active menu should stop any further processing of these if some are in progress. My idea in this PR is to simply "halt" a promise queue which means no further actions will be processed an no listeners will be notified - eventually the entire thing will be just garbage collected.
| function useStoreSavedMenuItem( query ) { | ||
| const { assignMenuItemIdToClientId } = useDispatch( | ||
| 'core/edit-navigation' | ||
| ); | ||
| const { receiveEntityRecords } = useDispatch( 'core' ); | ||
| const select = useSelect( ( s ) => s ); | ||
| return useCallback( | ||
| ( clientId, menuItem ) => { | ||
| assignMenuItemIdToClientId( query, menuItem.id, clientId ); | ||
| receiveEntityRecords( | ||
| 'root', | ||
| 'menuItem', | ||
| [ ...select( 'core' ).getMenuItems( query ), menuItem ], | ||
| query, | ||
| false | ||
| ); | ||
| }, | ||
| [ query ] | ||
| ); |
There was a problem hiding this comment.
I am not too happy about this part. Previously menuItems were kept in react ref. Now the core data layer is the primary source of truth. This means that creating a temporary menu item should be reflected in redux state. The only way to do it ATM is receiveEntityRecords which accepts an entire page of results. Maybe we should add an action like appendEntityRecord to avoid this select madness here?
| const menuItemsByClientId = mapMenuItemsByClientId( | ||
| select( 'core' ).getMenuItems( query ), | ||
| select( 'core/edit-navigation' ).getMenuItemIdToClientIdMapping( | ||
| query | ||
| ) | ||
| ); |
There was a problem hiding this comment.
saveBlocks is not called synchronously when the save button is pressed. Instead, we wait for all draft menu items to be created first, and only then saveBlocks is called. This means that we need a fresh version of menuItemId -> clientId mapping - if we simply selected it in line 42 above then it would be outdated by the time this function is called. Ditto for getMenuItems. I am not too happy about this solution, but the only alternative I can think of that would involve a "valid" use of useSelect calls is keeping a state variable like const [shouldSave, setShouldSave] = useState( false );. Then, when all the async operations are finished, we'd re-render the component using setShouldSave( true );, and react to this change in an effect which would then have access to the most recent version of select()-ed data.
| const post = createStubPost( query.menus, navigationBlock ); | ||
| const entityStored = receiveEntityRecords( | ||
| 'root', | ||
| 'postType', | ||
| post, | ||
| { id: post.id }, | ||
| false | ||
| ); | ||
| entityStored.then( () => { | ||
| setPostId( post.id ); | ||
| } ); |
There was a problem hiding this comment.
I am pretty sure this isn't the correct way to store this stub post - useEntityBlockEditor still sends a HTTP request in an attempt to fetch the data. Should this even be a post? Maybe a dedicated entity kind would work better?
| /** | ||
| * Block editor data store configuration. | ||
| * | ||
| * @see https://github.com/WordPress/gutenberg/blob/master/packages/data/README.md#registerStore | ||
| * | ||
| * @type {Object} | ||
| */ |
There was a problem hiding this comment.
I didn't pay too much attention to the comments yet, let it be - I will clean it all up later :-)
adamziel
changed the title
|
cc @noisysocks |
|
I am thinking we need to implement something that demonstrates the value of this refactor in order to gauge whether the extra bit of complexity is worth it or not. Could you maybe add a rough pass at undo/redo to this PR to see how it looks? That's what I imagine the primary value of using |
|
Fixing #22625 could be a good first implementation of an |
|
The functions in With that in mind, it's weird to me that this PR has Could we move the code that's in To illustrate, here's what I imagine you'd be able to do in the DevTools console: // Hits GET /wp/v2/menu-items, munges the data into Link blocks, and returns
// everything as a fake post.
const post = wp.data.select( 'edit-navigation' ).getNavigationPost( /* menuId: */ 123 );
console.log( post.id ); // navigation-post-123
console.log( post.blocks ); // [ { name: 'core/navigation', ... } ]
// Hits POST /wp/v2/menu-items once for every Link block that doesn't have an
// associated menu item. (IDK what a good name for this is.)
wp.data.dispatch( 'edit-navigation' ).createMissingMenuItems( post );
// Hits the Customiser endpoint and saves everything.
wp.data.dispatch( 'edit-navigation' ).saveNavigationPost( post );
// If we put our fake post into the 'core' registry then we can do things like
// undo and redo.
wp.data.dispatch( 'core' ).receiveEntityRecords( 'postType', 'post', [ post ] );
wp.data.dispatch( 'core' ).editEntityRecord(
'postType',
'post',
'navigation-post-123',
{ blocks: [
...post.blocks,
{ name: 'core/paragraph', content: 'This will be undone!', ... }
] }
);
wp.data.dispatch( 'core' ).undo();
wp.data.dispatch( 'core' ).redo();Does that make sense? Thoughts? |
TIL - that's a good mental model, thank you! I think exposing
@noisysocks while I agree the name My understanding is that in order to expose a An alternative approach would be storing |
|
@adamziel I think you're on the right path storing this in state, but a lot the comments were probably about making the public facing API friendlier rather than using an alternate solution like block attributes. The block editor store has a lot of relevance in that there are reducers that encapsulate things like mappings (block order, parent/child relationships), but they're not necessarily exposed to the user directly through actions. For example It might be that we have an In terms of That's another way to encapsulate the various working parts, but lets have more conversation on that. |
@talldan @adamziel #22428 fixes that, because when we delete an entity we don't need to use state anymore. I think @noisysocks 's suggestion with demo-ing undo/redo in the navigation screen is a better demonstration of this PR's powers! |
|
@noisysocks I explored getting rid of most hooks in favor of the actions, reducers, and such. Interestingly enough it ended up being quite similar to the API you proposed. I played with undo/redo and it seems to work 🎉 Even better - there is no more weirdness with passing We also now have a selector called @talldan I also ended up moving the mapping to There are a few minor bugs that I noticed, but let's have more discussion about the approach before I go ahead and start fixing them. |
| } | ||
| } | ||
|
|
||
| function serializeProcessing( callback ) { |
There was a problem hiding this comment.
This wrapper guarantees serial execution of data processing actions. saveNavigationPost() needs to wait for all the missing items to be created, and running multiple createMissingMenuItems() concurrently could result in sending more requests than required.
| yield dispatch( | ||
| 'core/notices', | ||
| 'createErrorNotice', | ||
| __( 'There was an error.' ), | ||
| { | ||
| type: 'snackbar', | ||
| } | ||
| ); |
There was a problem hiding this comment.
It would be nice to extract these notices outside of the save action, but that would require some way to wait for serialProcessing() to finish. I think it could wait until another iteration.
| ); | ||
|
|
||
| try { | ||
| yield* batchSave( menuId, menuItemsByClientId, post.blocks[ 0 ] ); |
There was a problem hiding this comment.
This try catch needs more attention to make sure all the errors are handled.
|
@draganescu You might be thinking of a different issue. #22625 is related to the |
That's interesting! What are the pros and cons for doing this versus storing the mapping in the |
There was a problem hiding this comment.
Heck yeah! Nice work. I'm feeling pretty good about this. It's exactly what I'd pictured, only with very clever Adam-isms like serializeProcessing() 😄
A few more notes in addition to the ones I wrote inline:
-
This PR makes the terminology we use in
edit-navigationa little all over the place. For example, we call the entity a "menu" in some function names (MenuEditor) but then a "navigation" in others (useNavigationBlockEditor()). We need to settle on a few key terms. I think what probably makes sense is: the navigation screen edits a navigation post, and the navigation store saves a navigation post as a collection of menus and menu items.- Semi-relatedly: is there anything we can do to make it clearer that a navigation post is not a "real" post that can be persisted and accessed via
/wp/v2/posts?
- Semi-relatedly: is there anything we can do to make it clearer that a navigation post is not a "real" post that can be persisted and accessed via
-
I think, as much as possible, let's make sure that consumers of
wp.data.select( 'core/edit-navigation' )andwp.data.dispatch( 'core/edit-navigation')don't need to care about menu items. They're an internal detail. For example, let's not export actions likestartProcessingMenuItems(). -
Since it's now easier to do, should we add undo/redo to the Navigation screen? Even just having the keyboard shortcuts would be useful.
Let me know when you're ready for me to review this with a finer comb.
| </div> | ||
| ); | ||
| } | ||
|
|
||
| const NavigationBlockEditorProvider = ( { |
There was a problem hiding this comment.
The name ending with Provider implies that this component only provides context when in fact it renders HTML as well. Maybe rename this component or, since the bulk of the magic happens in hooks and actions, consider putting everything back into MenuEditor?
| 'receiveEntityRecords', | ||
| 'root', | ||
| 'menuItem', | ||
| [ ...menuItems, menuItem ], |
There was a problem hiding this comment.
Why is this [ ...menuItems, menuItem ] and not menuItem? I thought that receiveEntityRecords( items ) will append items to whatever items are already in the store.
There was a problem hiding this comment.
We want to:
- Append another entry to the store
- Append entry ID to stored query results
Using just menuItem does 1) but breaks 2) as instead of appending it replaces the first stored ID. Using [ ...menuItems, menuItem ] both appends the new entry to queriedData.items AND appends entry ID to stored query results to queriedData.queries["menus=123"]. Ideally the entity store would be offer some sort of "append" action.
| yield dispatch( | ||
| 'core/edit-navigation', | ||
| 'startProcessingMenuItems', | ||
| menuId | ||
| ); |
There was a problem hiding this comment.
These actions don't strike me as particularly useful for third parties. We can emulate "private actions" by dispatching the action object directly here.
return {
type: 'START_PROCESSING_MENU_ITEMS',
menuId,
};| 'core/edit-navigation', | ||
| 'enqueueAfterProcessing', | ||
| menuId, | ||
| callback |
There was a problem hiding this comment.
Should we be storing non-serialisable things like functions in a Redux store? I imagine it may break tooling such as the Redux DevTools.
There was a problem hiding this comment.
We already store non-serializable things in other stores, e.g. EquivalentKeyMap instances. It's not a perfect solution, but it's good enough for starters. If any serialization issue comes up, it's entirely okay to just skip the processing part of the state tree.
|
|
||
| const store = registerStore( MODULE_KEY, { | ||
| ...storeConfig, | ||
| persist: [ 'preferences' ], |
There was a problem hiding this comment.
There is no preferences key in the reducer.
| export const buildNavigationPostId = ( menuId ) => | ||
| `navigation-post-${ menuId }`; | ||
|
|
||
| export function uuidv4() { |
There was a problem hiding this comment.
Should we use the uuid package instead? We already are using it in some packages e.g. @wordpress/blocks.
adamziel
force-pushed
the
try/use-store-for-edit-navigation-screen
branch
from
0552b25
to
592e0f5
Compare
|
I addressed all your feedback @noisysocks, this is ready for another round. Unfortunately it still attempts to request the post from the API - I will debug it tomorrow.
Probably not much other than naming. |
| export function getMenuItemToClientIdMapping( postId ) { | ||
| return { | ||
| type: 'GET_MENU_ITEM_TO_CLIENT_ID_MAPPING', | ||
| postId, | ||
| }; | ||
| } |
There was a problem hiding this comment.
I ended up moving this mapping back to state - keeping it in the post turned out to be troublesome (e.g. every update created additional undo actions). I'm not sure if there is any other way to have a "private selector" other than custom control - I'm open to refactoring this bit.
| }; | ||
| } | ||
|
|
||
| export function getNavigationPost( menuId ) { |
There was a problem hiding this comment.
getNavigationPost seems like the argument is a post id. Would getNavigationPostByMenuId be clearer?
There was a problem hiding this comment.
It should match the name of the selector. I'm not against renaming the selector to something like getNavigationPostForMenu( menuId ), though, if you want! 🙂
…e rogue http query)
adamziel
force-pushed
the
try/use-store-for-edit-navigation-screen
branch
from
ecbdf16
to
27eac24
Compare
Description
This PR replaces the component state like
const [blocks, setBlocks] = useState();andconst menuItemRef = useRef()in favor of the redux state and data layer.How has this been tested?
/wp-admin/admin.php?page=gutenberg-navigationand interact with it in every way you can think of: create a new menu, add menu items, edit existing ones, remove some, save the menu, rinse and repeat without refreshing the page.Types of changes
New feature
Checklist: